Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing

[M. Mohan Kumar]

-- 
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 <address@hidden>
> > > > ---
> > > > 
> > > >  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);
>     }
>