--
Regards,
M. Mohan Kumar
On Wednesday, October 12, 2011 09:37:23 PM Daniel P. Berrange wrote:
> On Wed, Oct 12, 2011 at 09:05:50PM +0530, M. Mohan Kumar wrote:
> > > On Wed, Oct 12, 2011 at 01:24:16PM +0530, M. Mohan Kumar wrote:
> > > > Security model is needed only for 'local' fs driver.
> > > >
> > > > Signed-off-by: M. Mohan Kumar <mo...@in.ibm.com>
> > > > ---
> > > >
> > > > fsdev/qemu-fsdev.c | 6 +----
> > > > fsdev/qemu-fsdev.h | 1 +
> > > > hw/9pfs/virtio-9p-device.c | 47
> > > > ++++++++++++++++++++++--------------------- vl.c
> > > >
> > > > | 20 +++++++++++++++--
> > > >
> > > > 4 files changed, 43 insertions(+), 31 deletions(-)
> > > >
> > > > --- a/fsdev/qemu-fsdev.h
> > > > +++ b/fsdev/qemu-fsdev.h
> > > > @@ -40,6 +40,7 @@ typedef struct FsTypeTable {
> > > >
> > > > typedef struct FsTypeEntry {
> > > >
> > > > char *fsdev_id;
> > > > char *path;
> > > >
> > > > + char *fsdriver;
> > > >
> > > > char *security_model;
> > > > int cache_flags;
> > > > FileOperations *ops;
> > > >
> > > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > > > index aac58ad..1846e36 100644
> > > > --- a/hw/9pfs/virtio-9p-device.c
> > > > +++ b/hw/9pfs/virtio-9p-device.c
> > > > @@ -83,29 +83,30 @@ VirtIODevice *virtio_9p_init(DeviceState *dev,
> > > > V9fsConf *conf)
> > > >
> > > > exit(1);
> > > >
> > > > }
> > > >
> > > > - if (!strcmp(fse->security_model, "passthrough")) {
> > > > - /* Files on the Fileserver set to client user credentials */
> > > > - s->ctx.fs_sm = SM_PASSTHROUGH;
> > > > - s->ctx.xops = passthrough_xattr_ops;
> > > > - } else if (!strcmp(fse->security_model, "mapped")) {
> > > > - /* Files on the fileserver are set to QEMU credentials.
> > > > - * Client user credentials are saved in extended attributes.
> > > > - */
> > > > - s->ctx.fs_sm = SM_MAPPED;
> > > > - s->ctx.xops = mapped_xattr_ops;
> > > > - } else if (!strcmp(fse->security_model, "none")) {
> > > > - /*
> > > > - * Files on the fileserver are set to QEMU credentials.
> > > > - */
> > > > - s->ctx.fs_sm = SM_NONE;
> > > > - s->ctx.xops = none_xattr_ops;
> > > > - } else {
> > > > - fprintf(stderr, "Default to security_model=none. You may
> > > > want" - " enable advanced security model using "
> > > > - "security option:\n\t security_model=passthrough\n\t
> > > > " - "security_model=mapped\n");
> > > > - s->ctx.fs_sm = SM_NONE;
> > > > - s->ctx.xops = none_xattr_ops;
> > > > + /* security models is needed only for local fs driver */
> > > > + if (!strcmp(fse->fsdriver, "local")) {
> > > > + if (!strcmp(fse->security_model, "passthrough")) {
> > > > + /* Files on the Fileserver set to client user
> > > > credentials */ + s->ctx.fs_sm = SM_PASSTHROUGH;
> > > > + s->ctx.xops = passthrough_xattr_ops;
> > > > + } else if (!strcmp(fse->security_model, "mapped")) {
> > > > + /* Files on the fileserver are set to QEMU credentials.
> > > > + * Client user credentials are saved in extended
> > > > attributes. + */
> > > > + s->ctx.fs_sm = SM_MAPPED;
> > > > + s->ctx.xops = mapped_xattr_ops;
> > > > + } else if (!strcmp(fse->security_model, "none")) {
> > > > + /*
> > > > + * Files on the fileserver are set to QEMU credentials.
> > > > + */
> > > > + s->ctx.fs_sm = SM_NONE;
> > > > + s->ctx.xops = none_xattr_ops;
> > > > + } else {
> > > > + fprintf(stderr, "Invalid security_model %s specified.\n"
> > > > + "Available security models are:\t "
> > > > + "passthrough,mapped or none\n",
> > > > fse->security_model); + exit(1);
> > > > + }
> > >
> > > Are you sure there aren't use cases where people would like to
> > > choose between passthrough & mapped, even when using the 'proxy'
> > > or 'handle' security drivers.
> >
> > Proxy FS driver is added to overcome the limit imposed by local +
> > passthrough security model combination that needs qemu to be started by
> > root user. Mapped and none secuiry model can be used by non root user
> > also.
> >
> > So Proxy FS driver does not need any security model(its pass-through
> > only)
>
> The Proxy FS driver does not "need" the security model, but if so desired
> it would be possible to choose to implement the security models. It just
> happens that the driver is hardcoded to only operate in 'passthrough'
> mode.
>
> I think that disabling the parsing of the 'security' parameter for
> non-local drivers is dangerous, because an application might think that
> the 'mapped' model was supported, but its parameter would get silently
> ignored. If the requested value is not supported, then the application
> should always be told about that.
>
> So, IMHO, it would be better to have logic such as:
Daniel,
Code in virtio-9p-device.c does the validation of security model. Aneesh's
recent patch moved the validation in fsdev/qemu-fsdev.c
>
> if (strcmp(security_mode, "passthrough") == 0) {
> ...
> } else if (strcmp(security_model, "mapped") == 0) {
> if (strcmp(fsdriver, "local") != 0) {
> fprintf(stderr, "security mode 'passthrough' is not supported
> by '%s'\n", fsdriver); exit(1);
> }
> ...
> } else if (strcmp(security_model, "none") == 0) {
> if (strcmp(fsdriver, "local") != 0) {
> fprintf(stderr, "security mode 'passthrough' is not supported
> by '%s'\n", fsdriver); exit(1);
> }
> ...
> } else {
> fprintf(stderr, "unknown security mode '%s'. valid options are
> passthrough, mapped, none\n", security_model); exit(1);
> }
>
