* [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
@ 2011-10-12 7:54 M. Mohan Kumar
2011-10-12 8:28 ` Daniel P. Berrange
2011-10-12 14:16 ` Aneesh Kumar K.V
0 siblings, 2 replies; 8+ messages in thread
From: M. Mohan Kumar @ 2011-10-12 7:54 UTC (permalink / raw)
To: qemu-devel
Security model is needed only for 'local' fs driver.
Signed-off-by: M. Mohan Kumar <mohan@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(-)
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 36db127..d08ba9c 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -58,11 +58,6 @@ int qemu_fsdev_add(QemuOpts *opts)
return -1;
}
- if (!sec_model) {
- fprintf(stderr, "fsdev: No security_model specified.\n");
- return -1;
- }
-
if (!path) {
fprintf(stderr, "fsdev: No path specified.\n");
return -1;
@@ -72,6 +67,7 @@ int qemu_fsdev_add(QemuOpts *opts)
fsle->fse.fsdev_id = g_strdup(fsdev_id);
fsle->fse.path = g_strdup(path);
+ fsle->fse.fsdriver = g_strdup(fstype);
fsle->fse.security_model = g_strdup(sec_model);
fsle->fse.ops = FsTypes[i].ops;
fsle->fse.cache_flags = 0;
diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
index 9c440f2..0f67880 100644
--- 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);
+ }
}
s->ctx.cache_flags = fse->cache_flags;
diff --git a/vl.c b/vl.c
index 6760e39..a961fa3 100644
--- a/vl.c
+++ b/vl.c
@@ -2795,6 +2795,7 @@ int main(int argc, char **argv, char **envp)
QemuOpts *fsdev;
QemuOpts *device;
const char *cache;
+ const char *fsdriver;
olist = qemu_find_opts("virtfs");
if (!olist) {
@@ -2809,13 +2810,26 @@ int main(int argc, char **argv, char **envp)
if (qemu_opt_get(opts, "fstype") == NULL ||
qemu_opt_get(opts, "mount_tag") == NULL ||
- qemu_opt_get(opts, "path") == NULL ||
- qemu_opt_get(opts, "security_model") == NULL) {
+ qemu_opt_get(opts, "path") == NULL) {
fprintf(stderr, "Usage: -virtfs fstype,path=/share_path/,"
- "security_model=[mapped|passthrough|none],"
+ "{security_model=[mapped|passthrough|none]},"
"mount_tag=tag.\n");
exit(1);
}
+ fsdriver = qemu_opt_get(opts, "fstype");
+ /* security model is mandatory for local fs driver */
+ if (!strcmp(fsdriver,"local") &&
+ !qemu_opt_get(opts,"security_model")) {
+ fprintf(stderr, "security model not specified for local"
+ " fs driver\n");
+ exit(1);
+ }
+ if (strcmp(fsdriver,"local") &&
+ qemu_opt_get(opts,"security_model")) {
+ fprintf(stderr, "security model is not needed for %s"
+ " fs driver\n", fsdriver);
+ exit(1);
+ }
fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
qemu_opt_get(opts, "mount_tag"), 1);
--
1.7.6
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
2011-10-12 7:54 [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing M. Mohan Kumar
@ 2011-10-12 8:28 ` Daniel P. Berrange
2011-10-12 14:22 ` Aneesh Kumar K.V
2011-10-12 15:35 ` M. Mohan Kumar
2011-10-12 14:16 ` Aneesh Kumar K.V
1 sibling, 2 replies; 8+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 8:28 UTC (permalink / raw)
To: M. Mohan Kumar; +Cc: qemu-devel
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 <mohan@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.
Both of the security models seem pretty generally useful to me,
regardless of the driver type.
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 :|
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
2011-10-12 8:28 ` Daniel P. Berrange
@ 2011-10-12 14:22 ` Aneesh Kumar K.V
2011-10-12 15:35 ` M. Mohan Kumar
1 sibling, 0 replies; 8+ messages in thread
From: Aneesh Kumar K.V @ 2011-10-12 14:22 UTC (permalink / raw)
To: Daniel P. Berrange, M. Mohan Kumar; +Cc: qemu-devel
On Wed, 12 Oct 2011 09:28:00 +0100, "Daniel P. Berrange" <berrange@redhat.com> 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 <mohan@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(-)
> >
....
> * 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.
Currently handle fs driver requires CAP_DAC_READ_SEARCH and if qemu is
not going to run with specific capabilities this implies root
privileges. So handle fs driver doesn't do the mapping required by
different security model. Proxy fs driver is enabling us to run file
system operations as "root". So even for that we don't need mapped
security model. Even if we want to store file attributes in xattr with
proxy fs driver, that will go as a proxy's argument not as -fsdev
argument. Proxy also don't require export path name. But that is another
patch.
>
> Both of the security models seem pretty generally useful to me,
> regardless of the driver type.
>
-aneesh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
2011-10-12 8:28 ` Daniel P. Berrange
2011-10-12 14:22 ` Aneesh Kumar K.V
@ 2011-10-12 15:35 ` M. Mohan Kumar
2011-10-12 16:07 ` Daniel P. Berrange
1 sibling, 1 reply; 8+ messages in thread
From: M. Mohan Kumar @ 2011-10-12 15:35 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel
--
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 <mohan@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)
> Both of the security models seem pretty generally useful to me,
> regardless of the driver type.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
2011-10-12 15:35 ` M. Mohan Kumar
@ 2011-10-12 16:07 ` Daniel P. Berrange
2011-10-14 4:24 ` M. Mohan Kumar
0 siblings, 1 reply; 8+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 16:07 UTC (permalink / raw)
To: M. Mohan Kumar; +Cc: qemu-devel
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 <mohan@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:
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 :|
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
2011-10-12 16:07 ` Daniel P. Berrange
@ 2011-10-14 4:24 ` M. Mohan Kumar
0 siblings, 0 replies; 8+ messages in thread
From: M. Mohan Kumar @ 2011-10-14 4:24 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel
--
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 <mohan@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);
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
2011-10-12 7:54 [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing M. Mohan Kumar
2011-10-12 8:28 ` Daniel P. Berrange
@ 2011-10-12 14:16 ` Aneesh Kumar K.V
1 sibling, 0 replies; 8+ messages in thread
From: Aneesh Kumar K.V @ 2011-10-12 14:16 UTC (permalink / raw)
To: M. Mohan Kumar, qemu-devel
On Wed, 12 Oct 2011 13:24:16 +0530, "M. Mohan Kumar" <mohan@in.ibm.com> wrote:
> Security model is needed only for 'local' fs driver.
Can you also cleanup that fstype -> fsdriver rename ? fsdriver seems
more appropriate.
>
> Signed-off-by: M. Mohan Kumar <mohan@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(-)
>
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index 36db127..d08ba9c 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -58,11 +58,6 @@ int qemu_fsdev_add(QemuOpts *opts)
> return -1;
> }
>
> - if (!sec_model) {
> - fprintf(stderr, "fsdev: No security_model specified.\n");
> - return -1;
> - }
> -
> if (!path) {
> fprintf(stderr, "fsdev: No path specified.\n");
> return -1;
> @@ -72,6 +67,7 @@ int qemu_fsdev_add(QemuOpts *opts)
>
> fsle->fse.fsdev_id = g_strdup(fsdev_id);
> fsle->fse.path = g_strdup(path);
> + fsle->fse.fsdriver = g_strdup(fstype);
Why use it as a string ? Why can't this again be an export_flag. That
would help us to avoid that strdup
> fsle->fse.security_model = g_strdup(sec_model);
> fsle->fse.ops = FsTypes[i].ops;
> fsle->fse.cache_flags = 0;
> diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
> index 9c440f2..0f67880 100644
> --- 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);
> + }
> }
>
> s->ctx.cache_flags = fse->cache_flags;
> diff --git a/vl.c b/vl.c
> index 6760e39..a961fa3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2795,6 +2795,7 @@ int main(int argc, char **argv, char **envp)
> QemuOpts *fsdev;
> QemuOpts *device;
> const char *cache;
> + const char *fsdriver;
>
> olist = qemu_find_opts("virtfs");
> if (!olist) {
> @@ -2809,13 +2810,26 @@ int main(int argc, char **argv, char **envp)
>
> if (qemu_opt_get(opts, "fstype") == NULL ||
> qemu_opt_get(opts, "mount_tag") == NULL ||
> - qemu_opt_get(opts, "path") == NULL ||
> - qemu_opt_get(opts, "security_model") == NULL) {
> + qemu_opt_get(opts, "path") == NULL) {
> fprintf(stderr, "Usage: -virtfs fstype,path=/share_path/,"
> - "security_model=[mapped|passthrough|none],"
> +
> "{security_model=[mapped|passthrough|none]},"
That should be
[security_model=....]
> "mount_tag=tag.\n");
> exit(1);
> }
> + fsdriver = qemu_opt_get(opts, "fstype");
> + /* security model is mandatory for local fs driver */
> + if (!strcmp(fsdriver,"local") &&
> + !qemu_opt_get(opts,"security_model")) {
> + fprintf(stderr, "security model not specified for local"
> + " fs driver\n");
> + exit(1);
> + }
> + if (strcmp(fsdriver,"local") &&
> + qemu_opt_get(opts,"security_model")) {
> + fprintf(stderr, "security model is not needed for %s"
> + " fs driver\n", fsdriver);
> + exit(1);
> + }
>
> fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
> qemu_opt_get(opts, "mount_tag"), 1);
Also needs a documentation update.
-aneesh
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing
@ 2011-10-14 12:06 M. Mohan Kumar
0 siblings, 0 replies; 8+ messages in thread
From: M. Mohan Kumar @ 2011-10-14 12:06 UTC (permalink / raw)
To: qemu-devel, aneesh.kumar
Except local fs driver other fs drivers (handle) don't need
security model. Update fsdev parameter parsing accordingly.
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
fsdev/qemu-fsdev.c | 26 +++++++++++++++++---------
qemu-options.hx | 12 ++++++++----
vl.c | 6 ++----
3 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index ce920d6..5977bcc 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -58,8 +58,15 @@ int qemu_fsdev_add(QemuOpts *opts)
return -1;
}
- if (!sec_model) {
- fprintf(stderr, "fsdev: No security_model specified.\n");
+ if (!strcmp(fsdriver, "local") && !sec_model) {
+ fprintf(stderr, "security model not specified, "
+ "local fs needs security model\nvalid options are:"
+ "\tsecurity_model=[passthrough|mapped|none]\n");
+ return -1;
+ }
+
+ if (strcmp(fsdriver, "local") && sec_model) {
+ fprintf(stderr, "only local fs driver needs security model\n");
return -1;
}
@@ -80,6 +87,10 @@ int qemu_fsdev_add(QemuOpts *opts)
}
}
+ if (strcmp(fsdriver, "local")) {
+ goto done;
+ }
+
if (!strcmp(sec_model, "passthrough")) {
fsle->fse.export_flags |= V9FS_SM_PASSTHROUGH;
} else if (!strcmp(sec_model, "mapped")) {
@@ -87,14 +98,11 @@ int qemu_fsdev_add(QemuOpts *opts)
} else if (!strcmp(sec_model, "none")) {
fsle->fse.export_flags |= V9FS_SM_NONE;
} 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");
-
- fsle->fse.export_flags |= V9FS_SM_NONE;
+ fprintf(stderr, "Invalid security model %s specified, valid options are"
+ "\n\t [passthrough|mapped|none]\n", sec_model);
+ return -1;
}
-
+done:
QTAILQ_INSERT_TAIL(&fsdriver_entries, fsle, next);
return 0;
}
diff --git a/qemu-options.hx b/qemu-options.hx
index 518a1f1..f05be30 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -527,13 +527,13 @@ DEFHEADING()
DEFHEADING(File system options:)
DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
- "-fsdev fsdriver,id=id,path=path,security_model=[mapped|passthrough|none]\n"
+ "-fsdev fsdriver,id=id,path=path,[security_model={mapped|passthrough|none}]\n"
" [,writeout=immediate]\n",
QEMU_ARCH_ALL)
STEXI
-@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},security_model=@var{security_model}[,writeout=@var{writeout}]
+@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}]
@findex -fsdev
Define a new file system device. Valid options are:
@table @option
@@ -555,7 +555,9 @@ attributes like uid, gid, mode bits and link target are stored as
file attributes. Directories exported by this security model cannot
interact with other unix tools. "none" security model is same as
passthrough except the sever won't report failures if it fails to
-set file attributes like ownership.
+set file attributes like ownership. Security model is mandatory
+only for local fsdriver. Other fsdrivers (like handle) don't take
+security model as a parameter.
@item writeout=@var{writeout}
This is an optional argument. The only supported value is "immediate".
This means that host page cache will be used to read and write data but
@@ -609,7 +611,9 @@ attributes like uid, gid, mode bits and link target are stored as
file attributes. Directories exported by this security model cannot
interact with other unix tools. "none" security model is same as
passthrough except the sever won't report failures if it fails to
-set file attributes like ownership.
+set file attributes like ownership. Security model is mandatory only
+for local fsdriver. Other fsdrivers (like handle) don't take security
+model as a parameter.
@item writeout=@var{writeout}
This is an optional argument. The only supported value is "immediate".
This means that host page cache will be used to read and write data but
diff --git a/vl.c b/vl.c
index 3b8199f..d672268 100644
--- a/vl.c
+++ b/vl.c
@@ -2800,14 +2800,12 @@ int main(int argc, char **argv, char **envp)
if (qemu_opt_get(opts, "fsdriver") == NULL ||
qemu_opt_get(opts, "mount_tag") == NULL ||
- qemu_opt_get(opts, "path") == NULL ||
- qemu_opt_get(opts, "security_model") == NULL) {
+ qemu_opt_get(opts, "path") == NULL) {
fprintf(stderr, "Usage: -virtfs fsdriver,path=/share_path/,"
- "security_model=[mapped|passthrough|none],"
+ "[security_model={mapped|passthrough|none}],"
"mount_tag=tag.\n");
exit(1);
}
-
fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
qemu_opt_get(opts, "mount_tag"), 1);
if (!fsdev) {
--
1.7.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-10-14 12:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-12 7:54 [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing M. Mohan Kumar
2011-10-12 8:28 ` Daniel P. Berrange
2011-10-12 14:22 ` Aneesh Kumar K.V
2011-10-12 15:35 ` M. Mohan Kumar
2011-10-12 16:07 ` Daniel P. Berrange
2011-10-14 4:24 ` M. Mohan Kumar
2011-10-12 14:16 ` Aneesh Kumar K.V
-- strict thread matches above, loose matches on Subject: below --
2011-10-14 12:06 M. Mohan Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).