From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REZJd-0006s1-ND for qemu-devel@nongnu.org; Fri, 14 Oct 2011 00:24:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1REZJc-0002c8-Gv for qemu-devel@nongnu.org; Fri, 14 Oct 2011 00:24:13 -0400 Received: from e28smtp03.in.ibm.com ([122.248.162.3]:39193) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REZJb-0002bk-Pu for qemu-devel@nongnu.org; Fri, 14 Oct 2011 00:24:12 -0400 Received: from d28relay01.in.ibm.com (d28relay01.in.ibm.com [9.184.220.58]) by e28smtp03.in.ibm.com (8.14.4/8.13.1) with ESMTP id p9E4O58H018190 for ; Fri, 14 Oct 2011 09:54:05 +0530 Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p9E4O4LU4497622 for ; Fri, 14 Oct 2011 09:54:04 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p9E4O4uT003765 for ; Fri, 14 Oct 2011 15:24:04 +1100 From: "M. Mohan Kumar" Date: Fri, 14 Oct 2011 09:54:00 +0530 References: <1318406056-17026-1-git-send-email-mohan@in.ibm.com> <201110122105.50763.mohan@in.ibm.com> <20111012160723.GH9848@redhat.com> In-Reply-To: <20111012160723.GH9848@redhat.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201110140954.00745.mohan@in.ibm.com> Subject: Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org -- 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 > > > > --- > > > > > > > > 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); > } >