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

[Daniel P. Berrange]

On Wed, Oct 12, 2011 at 09:05:50PM +0530, M. Mohan Kumar wrote:
> -- 
> Regards,
> M. Mohan Kumar
> On Wednesday, October 12, 2011 01:58:00 PM Daniel P. Berrange 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:

    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);
    }
  

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|