Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Tue, Mar 03, 2009 at 09:08:20AM -0500, Daniel J Walsh wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA1 > > Daniel P. Berrange wrote: > > On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote: > >> -BEGIN PGP SIGNED MESSAGE- > >> Hash: SHA1 > >> > >> Another patch off latest repository. > >> > >> This patch does not require the XML to include a label, although this is > >> still supported. > >> > >> Implemented most of the comments from Jim. make check and make > >> syntax-check passes, Added seclabeltest.c to run in tests, Updated > >> capability.rng, although not really sure I did it right. > >> > >> This patch will generate random MCS Labels and relabels the image files > >> to match. Seems to work well on F11. > > > > I had a few problems with label generation on my F11 machine - perhaps > > you have a newer version of the patch than the one I applied. > > > > I found I need the following additional patch.. > > > > - Make domainGenSecurityLabel() give diagnostics for each type of error > >instead of using generic error message in caller > > - Change logic bug 'c1 == c2' to 'c1 < c2' > > - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with > >the form "c210,c502" only wanting "c210.c502" > This does not make sense. c210,c502 is valid. c210.c502 means include > the range. c210, c211, c212...c502. Hmm, I reverted this bit of the change & it works fine now - it was probably one of the other bugs (or outdated RPMs) that caused the original failure Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Daniel P. Berrange wrote: > On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote: >> -BEGIN PGP SIGNED MESSAGE- >> Hash: SHA1 >> >> Another patch off latest repository. >> >> This patch does not require the XML to include a label, although this is >> still supported. >> >> Implemented most of the comments from Jim. make check and make >> syntax-check passes, Added seclabeltest.c to run in tests, Updated >> capability.rng, although not really sure I did it right. >> >> This patch will generate random MCS Labels and relabels the image files >> to match. Seems to work well on F11. > > I had a few problems with label generation on my F11 machine - perhaps > you have a newer version of the patch than the one I applied. > > I found I need the following additional patch.. > > - Make domainGenSecurityLabel() give diagnostics for each type of error >instead of using generic error message in caller > - Change logic bug 'c1 == c2' to 'c1 < c2' > - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with >the form "c210,c502" only wanting "c210.c502" This does not make sense. c210,c502 is valid. c210.c502 means include the range. c210, c211, c212...c502. > - Fix use of STREQ - no need for == 0 in there I am reworking this code to use INT instead of strings. > - Use VIR_FREE/VIR_ALLOC for memory mgmt > > With this I can successfully start several VMs, and see them all > using different contexts, and see the files labelled > > # ps -xZ | grep qemu | awk '{print $1}' > system_u:system_r:qemu_t:s0:c35.c537 > system_u:system_r:qemu_t:s0:c210.c502 > > # ls -Zl /var/lib/libvirt/images/ > total 504 > -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c210.c502 root root > 1073741824 2009-03-03 12:15 demo2.img > -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c35.c537 root root 1073741824 > 2009-03-03 11:49 demo.img > > > Daniel > > Index: src/qemu_driver.c > === > RCS file: /data/cvs/libvirt/src/qemu_driver.c,v > retrieving revision 1.212 > diff -u -p -r1.212 qemu_driver.c > --- src/qemu_driver.c 3 Mar 2009 12:03:44 - 1.212 > +++ src/qemu_driver.c 3 Mar 2009 12:25:47 - > @@ -1316,13 +1316,11 @@ static int qemudStartVMDaemon(virConnect > > /* If you are using a SecurityDriver and there was no security label in >database, then generate a security label for isolation */ > -if (vm->def->seclabel.label == NULL && driver->securityDriver) { > -if (driver->securityDriver->domainGenSecurityLabel(vm) < 0) { > -qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, > - "%s", _("Unable to generate Security Label")); > -return -1; > -} > -} > +if (vm->def->seclabel.label == NULL && > + driver->securityDriver && > + driver->securityDriver->domainGenSecurityLabel && > + driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) > +return -1; > > FD_ZERO(&keepfd); > > Index: src/security.h > === > RCS file: /data/cvs/libvirt/src/security.h,v > retrieving revision 1.1 > diff -u -p -r1.1 security.h > --- src/security.h3 Mar 2009 09:44:42 - 1.1 > +++ src/security.h3 Mar 2009 12:25:47 - > @@ -37,7 +37,8 @@ typedef int (*virSecurityDomainRestoreIm > typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn, > virDomainObjPtr vm, > virDomainDeviceDefPtr dev); > -typedef int (*virSecurityDomainGenLabel) (virDomainObjPtr sec); > +typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn, > + virDomainObjPtr sec); > typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn, >virDomainObjPtr vm, >virSecurityLabelPtr sec); > Index: src/security_selinux.c > === > RCS file: /data/cvs/libvirt/src/security_selinux.c,v > retrieving revision 1.1 > diff -u -p -r1.1 security_selinux.c > --- src/security_selinux.c3 Mar 2009 10:06:49 - 1.1 > +++ src/security_selinux.c3 Mar 2009 12:25:47 - > @@ -24,6 +24,9 @@ > #include "util.h" > #include "memory.h" > > + > +#define VIR_FROM_THIS VIR_FROM_SECURITY > + > static char default_domain_context[1024]; > static char default_image_context[1024]; > #define SECURITY_SELINUX_VOID_DOI "0" > @@ -45,10 +48,11 @@ mcsAdd(const char *mcs) > struct MCS *ptr; > > for (ptr = mcsList; ptr; ptr = ptr->next) { > -if (STREQ(ptr->mcs, mcs) == 0) > +if (STREQ(ptr->mcs, mcs)) > return -1; > } > -ptr = malloc(sizeof(struct MCS)); > +if (VIR_ALLOC(ptr) < 0) > +return
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA1 > > Another patch off latest repository. > > This patch does not require the XML to include a label, although this is > still supported. > > Implemented most of the comments from Jim. make check and make > syntax-check passes, Added seclabeltest.c to run in tests, Updated > capability.rng, although not really sure I did it right. > > This patch will generate random MCS Labels and relabels the image files > to match. Seems to work well on F11. I had a few problems with label generation on my F11 machine - perhaps you have a newer version of the patch than the one I applied. I found I need the following additional patch.. - Make domainGenSecurityLabel() give diagnostics for each type of error instead of using generic error message in caller - Change logic bug 'c1 == c2' to 'c1 < c2' - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with the form "c210,c502" only wanting "c210.c502" - Fix use of STREQ - no need for == 0 in there - Use VIR_FREE/VIR_ALLOC for memory mgmt With this I can successfully start several VMs, and see them all using different contexts, and see the files labelled # ps -xZ | grep qemu | awk '{print $1}' system_u:system_r:qemu_t:s0:c35.c537 system_u:system_r:qemu_t:s0:c210.c502 # ls -Zl /var/lib/libvirt/images/ total 504 -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c210.c502 root root 1073741824 2009-03-03 12:15 demo2.img -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c35.c537 root root 1073741824 2009-03-03 11:49 demo.img Daniel Index: src/qemu_driver.c === RCS file: /data/cvs/libvirt/src/qemu_driver.c,v retrieving revision 1.212 diff -u -p -r1.212 qemu_driver.c --- src/qemu_driver.c 3 Mar 2009 12:03:44 - 1.212 +++ src/qemu_driver.c 3 Mar 2009 12:25:47 - @@ -1316,13 +1316,11 @@ static int qemudStartVMDaemon(virConnect /* If you are using a SecurityDriver and there was no security label in database, then generate a security label for isolation */ -if (vm->def->seclabel.label == NULL && driver->securityDriver) { -if (driver->securityDriver->domainGenSecurityLabel(vm) < 0) { -qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to generate Security Label")); -return -1; -} -} +if (vm->def->seclabel.label == NULL && + driver->securityDriver && + driver->securityDriver->domainGenSecurityLabel && + driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) +return -1; FD_ZERO(&keepfd); Index: src/security.h === RCS file: /data/cvs/libvirt/src/security.h,v retrieving revision 1.1 diff -u -p -r1.1 security.h --- src/security.h 3 Mar 2009 09:44:42 - 1.1 +++ src/security.h 3 Mar 2009 12:25:47 - @@ -37,7 +37,8 @@ typedef int (*virSecurityDomainRestoreIm typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev); -typedef int (*virSecurityDomainGenLabel) (virDomainObjPtr sec); +typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn, + virDomainObjPtr sec); typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn, virDomainObjPtr vm, virSecurityLabelPtr sec); Index: src/security_selinux.c === RCS file: /data/cvs/libvirt/src/security_selinux.c,v retrieving revision 1.1 diff -u -p -r1.1 security_selinux.c --- src/security_selinux.c 3 Mar 2009 10:06:49 - 1.1 +++ src/security_selinux.c 3 Mar 2009 12:25:47 - @@ -24,6 +24,9 @@ #include "util.h" #include "memory.h" + +#define VIR_FROM_THIS VIR_FROM_SECURITY + static char default_domain_context[1024]; static char default_image_context[1024]; #define SECURITY_SELINUX_VOID_DOI "0" @@ -45,10 +48,11 @@ mcsAdd(const char *mcs) struct MCS *ptr; for (ptr = mcsList; ptr; ptr = ptr->next) { -if (STREQ(ptr->mcs, mcs) == 0) +if (STREQ(ptr->mcs, mcs)) return -1; } -ptr = malloc(sizeof(struct MCS)); +if (VIR_ALLOC(ptr) < 0) +return -1; ptr->mcs = strdup(mcs); ptr->next = mcsList; mcsList = ptr; @@ -62,7 +66,7 @@ mcsRemove(const char *mcs) struct MCS *ptr = NULL; for (ptr = mcsList; ptr; ptr = ptr->next) { -if (STREQ(ptr->mcs, mcs) == 0) { +if (STREQ(ptr->mcs, mcs)) { if (prevptr) prevptr->next = ptr->next; else { @@ -149,7 +153,8 @@ SELinuxInitialize(virConnectPtr conn) }
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA1 > > Another patch off latest repository. > > This patch does not require the XML to include a label, although this is > still supported. > > Implemented most of the comments from Jim. make check and make > syntax-check passes, Added seclabeltest.c to run in tests, Updated > capability.rng, although not really sure I did it right. > > This patch will generate random MCS Labels and relabels the image files > to match. Seems to work well on F11. > > I will back port some policy to allow it to work on F10. > > I think we need a mechanism in libvirtd.conf to turn this off. And > allow perhaps three modes. > > svirt=Disabled. No Security Driver. > svirt=MLS (Requires context in xml, no relabel of disks) > svirt=Standard, (If no XML label, then random generate one and reset > file context). > > How should I read config from libvirt.conf and and not enable he > SecurityModel? > > http://people.fedoraproject.org/~dwalsh/SELinux/svirt.patch I have finally applied this patch. I broke it up into a series of 7 patches across the different functional areas, to make it easier to bisect individual changes, so I applied it in the following pieces - Public API definitions - Internal driver API glue - Remote protocol API & glue - Core security driver infrastructure - Virsh additions for dominfo - SElinux security driver - QEMU integration with security driver I made a couple of small changes along the way... - virSecurityDriverStartup() takes a driver name, so HV drivers can explicitly configure which sec driver they want, overriding the default probed order. 'none' disables it completely - /etc/libvirt/qemu.conf gains a security_driver='XXX' config param accepting 'none' or 'selinux' to choose drivers. If not set it will probe for a driver, thus defaulting to SELinux if availab.e - Fixed the RNG schema for capabilities & domain XML format additions - Added a configure.in check for selinux_virtual_domain_context_path() and selinux_virtual_image_context_path() and make it disable the SELinux driver if these aren't found. These functions are new on F11, so we don't want to break build on RHEL-5 & earlier Fedora. I still think we need one further tweak to the XML. We have the ability to turn on / off of the security driver in QEMU, but I think we need better support for the automatic label generation. The current logic is doing - If is element in the XML, use that - Else generate a seclabel when starting a VM The trouble is when you then query the XML for a guest, you have no way of telling whether the is showing a generated one, or a predefined one. And if you dump and then reload the XML, your VM that used to be using a generated label, now gets fixed to that current label forever. This has caused us a great deal of pain in the past with generated VNC ports, and generated TAP device names. So I think we need to add an XML attribute to explicitly note that the label is generated eg, add type="static|dynamic" to ... system_u:system_r:qemu_t:s0:c210.c502 Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
Daniel P. Berrange wrote: > On Mon, Mar 02, 2009 at 09:18:05AM +1100, James Morris wrote: >> On Fri, 27 Feb 2009, Daniel J Walsh wrote: >> >>> I think we need a mechanism in libvirtd.conf to turn this off. And >>> allow perhaps three modes. >>> >>> svirt=Disabled. No Security Driver. >>> svirt=MLS (Requires context in xml, no relabel of disks) >>> svirt=Standard, (If no XML label, then random generate one and reset >>> file context). >> I wouldn't call these MLS and Standard. The simple isolation scheme with >> automatic labeling is just one way to do things. Down the track, we'll >> want to be able to specify arbitrary types for guests, not just for MLS. > > I think perhaps we should make this a QEMU driver config option (ie be > in /etc/libvirt/qemu.conf) and have 2 flags > > security_driver="selinux|none" > security_autolabel="yes|no" > > If security_autolabel is set to 'no', then the app must pass an explicit > security context in the domain XML, otherwise the domain is unconfined. > > If security_autolabel is set to 'yes', then if the app passes an explicit > security context this is used, otherwise it will auto-generate one at > startup of the VM. > Would we just use capabilities to communicate this choice? If so, would it be in the host section, or driver specific? Thanks, Cole -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Mon, Mar 02, 2009 at 09:18:05AM +1100, James Morris wrote: > On Fri, 27 Feb 2009, Daniel J Walsh wrote: > > > I think we need a mechanism in libvirtd.conf to turn this off. And > > allow perhaps three modes. > > > > svirt=Disabled. No Security Driver. > > svirt=MLS (Requires context in xml, no relabel of disks) > > svirt=Standard, (If no XML label, then random generate one and reset > > file context). > > I wouldn't call these MLS and Standard. The simple isolation scheme with > automatic labeling is just one way to do things. Down the track, we'll > want to be able to specify arbitrary types for guests, not just for MLS. I think perhaps we should make this a QEMU driver config option (ie be in /etc/libvirt/qemu.conf) and have 2 flags security_driver="selinux|none" security_autolabel="yes|no" If security_autolabel is set to 'no', then the app must pass an explicit security context in the domain XML, otherwise the domain is unconfined. If security_autolabel is set to 'yes', then if the app passes an explicit security context this is used, otherwise it will auto-generate one at startup of the VM. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Fri, 27 Feb 2009, Daniel J Walsh wrote: > I think we need a mechanism in libvirtd.conf to turn this off. And > allow perhaps three modes. > > svirt=Disabled. No Security Driver. > svirt=MLS (Requires context in xml, no relabel of disks) > svirt=Standard, (If no XML label, then random generate one and reset > file context). I wouldn't call these MLS and Standard. The simple isolation scheme with automatic labeling is just one way to do things. Down the track, we'll want to be able to specify arbitrary types for guests, not just for MLS. -- James Morris -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA1 > > Another patch off latest repository. > > This patch does not require the XML to include a label, although this is > still supported. > > Implemented most of the comments from Jim. make check and make > syntax-check passes, Added seclabeltest.c to run in tests, Updated > capability.rng, although not really sure I did it right. > > This patch will generate random MCS Labels and relabels the image files > to match. Seems to work well on F11. > > I will back port some policy to allow it to work on F10. > > I think we need a mechanism in libvirtd.conf to turn this off. And > allow perhaps three modes. > > svirt=Disabled. No Security Driver. > svirt=MLS (Requires context in xml, no relabel of disks) > svirt=Standard, (If no XML label, then random generate one and reset > file context). > > How should I read config from libvirt.conf and and not enable he > SecurityModel? libvirtd.conf is for the general daemon configuration. The QEMU driver has a separate /etc/libvirt/qemu.conf which is read by qemudLoadDriverConfig() into struct qemud_driver. The code for this is in src/qemu_conf.c The security driver is really more of a generic resource though, that we could use in several drivers, so another alternative is an /etc/libvirt/svirt.conf loaded the svirt driver. In any case look at the qemudLoadDriverConfig() for example of the API usage for config files Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Another patch off latest repository. This patch does not require the XML to include a label, although this is still supported. Implemented most of the comments from Jim. make check and make syntax-check passes, Added seclabeltest.c to run in tests, Updated capability.rng, although not really sure I did it right. This patch will generate random MCS Labels and relabels the image files to match. Seems to work well on F11. I will back port some policy to allow it to work on F10. I think we need a mechanism in libvirtd.conf to turn this off. And allow perhaps three modes. svirt=Disabled. No Security Driver. svirt=MLS (Requires context in xml, no relabel of disks) svirt=Standard, (If no XML label, then random generate one and reset file context). How should I read config from libvirt.conf and and not enable he SecurityModel? http://people.fedoraproject.org/~dwalsh/SELinux/svirt.patch -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmoTyMACgkQrlYvE4MpobPuHwCgkJqZenEwCWov96tTv+h3x8ec wmEAoMecJotrN009adtO3JOmkNLR3uXN =waHN -END PGP SIGNATURE- -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
On Fri, Feb 20, 2009 at 02:38:05PM +, Daniel P. Berrange wrote: > I think we need to move place where we set the exec context to after > the fork() call, ideally to be the very last call made before the > actual execve(). > > We do not currently have an easy way todo this, but I have the exact > same problem in my patches to integrate with cgroups - I need to add > the new PID to the appropriate cgroup immediately before exec'ing. > So i suggest the following patch whichs a generic callback to the > virExec() call, so we can implant the neccessary logic after the fork() > and just before the real execve(), and safely in the child process. > > To use this, we'd make qemudStartVM() pass in a virExecHook callback > which does the call to qemudDomainSetSecurityLabel() I've committed this patch to CVS now. > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -291,6 +291,7 @@ virEnumToString; > virEventAddHandle; > virEventRemoveHandle; > virExec; > +virExecWithHook; > virSetNonBlock; > virFormatMacAddr; > virGetHostname; > diff --git a/src/util.c b/src/util.c > --- a/src/util.c > +++ b/src/util.c > @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn, >const fd_set *keepfd, >pid_t *retpid, >int infd, int *outfd, int *errfd, > - int flags) { > + int flags, > + virExecHook hook, > + void *data) > +{ > pid_t pid; > int null, i, openmax; > int pipeout[2] = {-1,-1}; > @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn, > childerr != childout) > close(childerr); > > +if (hook) > +(hook)(data); > + > if (envp) > execve(argv[0], (char **) argv, (char**)envp); > else > @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn, > } > > int > -virExec(virConnectPtr conn, > -const char *const*argv, > -const char *const*envp, > -const fd_set *keepfd, > -pid_t *retpid, > -int infd, int *outfd, int *errfd, > -int flags) { > +virExecWithHook(virConnectPtr conn, > +const char *const*argv, > +const char *const*envp, > +const fd_set *keepfd, > +pid_t *retpid, > +int infd, int *outfd, int *errfd, > +int flags, > +virExecHook hook, > +void *data) > +{ > char *argv_str; > > if ((argv_str = virArgvToString(argv)) == NULL) { > @@ -462,7 +471,21 @@ virExec(virConnectPtr conn, > VIR_FREE(argv_str); > > return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, > - flags); > + flags, hook, data); > +} > + > +int > +virExec(virConnectPtr conn, > +const char *const*argv, > +const char *const*envp, > +const fd_set *keepfd, > +pid_t *retpid, > +int infd, int *outfd, int *errfd, > +int flags) > +{ > +return virExecWithHook(conn, argv, envp, keepfd, retpid, > + infd, outfd, errfd, > + flags, NULL, NULL); > } > > static int > @@ -580,7 +603,7 @@ virRun(virConnectPtr conn, > > if ((execret = __virExec(conn, argv, NULL, NULL, > &childpid, -1, &outfd, &errfd, > - VIR_EXEC_NONE)) < 0) { > + VIR_EXEC_NONE, NULL, NULL)) < 0) { > ret = execret; > goto error; > } > diff --git a/src/util.h b/src/util.h > --- a/src/util.h > +++ b/src/util.h > @@ -40,6 +40,21 @@ enum { > > int virSetNonBlock(int fd); > > +/* This will execute in the context of the first child > + * immediately after fork() */ > +typedef int (*virExecHook)(void *data); > + > +int virExecWithHook(virConnectPtr conn, > +const char *const*argv, > +const char *const*envp, > +const fd_set *keepfd, > +int *retpid, > +int infd, > +int *outfd, > +int *errfd, > +int flags, > +virExecHook hook, > +void *data); > int virExec(virConnectPtr conn, > const char *const*argv, > const char *const*envp, > > Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Jim Meyering wrote: > Daniel P. Berrange wrote: >> Just spotted one serious problem we need to address. The >> method 'qemudStartVMDaemon' quoted here is where we set the >> security label: > ... > > Good catch. > >> To use this, we'd make qemudStartVM() pass in a virExecHook callback >> which does the call to qemudDomainSetSecurityLabel() >> >> Daniel >> >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -291,6 +291,7 @@ virEnumToString; >> virEventAddHandle; >> virEventRemoveHandle; >> virExec; >> +virExecWithHook; > > Looks right to me. > ACK. This patch looks good to me and I have already rebased the svirt patch to match, waiting for this to get applied, then I will resubmit my patch. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmiqhYACgkQrlYvE4MpobMkFACeMWEmQfwKMe4Cn7NNikPk9f3+ zlMAoK7DeqbpTKho4Kw/bZtuA86vtMJK =eVSb -END PGP SIGNATURE- -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
Daniel P. Berrange wrote: > Just spotted one serious problem we need to address. The > method 'qemudStartVMDaemon' quoted here is where we set the > security label: ... Good catch. > To use this, we'd make qemudStartVM() pass in a virExecHook callback > which does the call to qemudDomainSetSecurityLabel() > > Daniel > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -291,6 +291,7 @@ virEnumToString; > virEventAddHandle; > virEventRemoveHandle; > virExec; > +virExecWithHook; Looks right to me. ACK. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Daniel P. Berrange wrote: > Just spotted one serious problem we need to address. The > method 'qemudStartVMDaemon' quoted here is where we set the > security label: > > On Tue, Feb 17, 2009 at 11:20:17AM -0500, Daniel J Walsh wrote: >> @@ -1178,6 +1237,16 @@ static int qemudStartVMDaemon(virConnect >> return -1; >> } >> >> +/* >> + * Set up the security label for the domain here, before doing >> + * too much else. >> + */ >> +if (qemudDomainSetSecurityLabel(conn, driver, vm) < 0) { >> +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, >> + _("Failed to set security label")); >> +return -1; >> +} >> + >> if (qemudExtractVersionInfo(emulator, >> NULL, >> &qemuCmdFlags) < 0) { > > > > Which ultimately calls the following method, which sets the context > to use for the next exec call. > >> +static int >> +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn, >> + virSecurityDriverPtr drv, >> + const virSecurityLabelDefPtr secdef) >> +{ >> +/* TODO: verify DOI */ >> + >> +if (!STREQ(drv->name, secdef->model)) { >> +virSecurityReportError(conn, VIR_ERR_ERROR, >> + _("%s: security label driver mismatch: " >> + "\'%s\' model configured for domain, but " >> + "hypervisor driver is \'%s\'."), >> + __func__, secdef->model, drv->name); >> +return -1; >> +} >> + >> +if (setexeccon(secdef->label) == -1) { >> +virSecurityReportError(conn, VIR_ERR_ERROR, >> + _("%s: unable to set security context " >> + "'\%s\': %s."), __func__, secdef->label, >> + strerror(errno)); >> +return -1; >> +} >> +return 0; >> +} > > The problem is that between the point where we set the exec context, > and the place where QEMU is finally exec'd, there is scope for several > other programs to be exec'd. Also there are other threads running > concurrently, and I'm under whether 'setexeccon' scope is per thread > or per process. > > I think we need to move place where we set the exec context to after > the fork() call, ideally to be the very last call made before the > actual execve(). > > We do not currently have an easy way todo this, but I have the exact > same problem in my patches to integrate with cgroups - I need to add > the new PID to the appropriate cgroup immediately before exec'ing. > So i suggest the following patch whichs a generic callback to the > virExec() call, so we can implant the neccessary logic after the fork() > and just before the real execve(), and safely in the child process. > > To use this, we'd make qemudStartVM() pass in a virExecHook callback > which does the call to qemudDomainSetSecurityLabel() > > Daniel > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -291,6 +291,7 @@ virEnumToString; > virEventAddHandle; > virEventRemoveHandle; > virExec; > +virExecWithHook; > virSetNonBlock; > virFormatMacAddr; > virGetHostname; > diff --git a/src/util.c b/src/util.c > --- a/src/util.c > +++ b/src/util.c > @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn, >const fd_set *keepfd, >pid_t *retpid, >int infd, int *outfd, int *errfd, > - int flags) { > + int flags, > + virExecHook hook, > + void *data) > +{ > pid_t pid; > int null, i, openmax; > int pipeout[2] = {-1,-1}; > @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn, > childerr != childout) > close(childerr); > > +if (hook) > +(hook)(data); > + > if (envp) > execve(argv[0], (char **) argv, (char**)envp); > else > @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn, > } > > int > -virExec(virConnectPtr conn, > -const char *const*argv, > -const char *const*envp, > -const fd_set *keepfd, > -pid_t *retpid, > -int infd, int *outfd, int *errfd, > -int flags) { > +virExecWithHook(virConnectPtr conn, > +const char *const*argv, > +const char *const*envp, > +const fd_set *keepfd, > +pid_t *retpid, > +int infd, int *outfd, int *errfd, > +int flags, > +virExecHook hook, > +void *data) > +{ > char *argv_str; > > if ((argv_str = virArgvToString(argv)) == NULL) { > @@ -462,7 +471,21 @@ virExec(virConnectPtr conn, > VIR_FREE(argv_str); > > return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, e
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
Just spotted one serious problem we need to address. The method 'qemudStartVMDaemon' quoted here is where we set the security label: On Tue, Feb 17, 2009 at 11:20:17AM -0500, Daniel J Walsh wrote: > @@ -1178,6 +1237,16 @@ static int qemudStartVMDaemon(virConnect > return -1; > } > > +/* > + * Set up the security label for the domain here, before doing > + * too much else. > + */ > +if (qemudDomainSetSecurityLabel(conn, driver, vm) < 0) { > +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, > + _("Failed to set security label")); > +return -1; > +} > + > if (qemudExtractVersionInfo(emulator, > NULL, > &qemuCmdFlags) < 0) { Which ultimately calls the following method, which sets the context to use for the next exec call. > +static int > +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn, > + virSecurityDriverPtr drv, > + const virSecurityLabelDefPtr secdef) > +{ > +/* TODO: verify DOI */ > + > +if (!STREQ(drv->name, secdef->model)) { > +virSecurityReportError(conn, VIR_ERR_ERROR, > + _("%s: security label driver mismatch: " > + "\'%s\' model configured for domain, but " > + "hypervisor driver is \'%s\'."), > + __func__, secdef->model, drv->name); > +return -1; > +} > + > +if (setexeccon(secdef->label) == -1) { > +virSecurityReportError(conn, VIR_ERR_ERROR, > + _("%s: unable to set security context " > + "'\%s\': %s."), __func__, secdef->label, > + strerror(errno)); > +return -1; > +} > +return 0; > +} The problem is that between the point where we set the exec context, and the place where QEMU is finally exec'd, there is scope for several other programs to be exec'd. Also there are other threads running concurrently, and I'm under whether 'setexeccon' scope is per thread or per process. I think we need to move place where we set the exec context to after the fork() call, ideally to be the very last call made before the actual execve(). We do not currently have an easy way todo this, but I have the exact same problem in my patches to integrate with cgroups - I need to add the new PID to the appropriate cgroup immediately before exec'ing. So i suggest the following patch whichs a generic callback to the virExec() call, so we can implant the neccessary logic after the fork() and just before the real execve(), and safely in the child process. To use this, we'd make qemudStartVM() pass in a virExecHook callback which does the call to qemudDomainSetSecurityLabel() Daniel diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -291,6 +291,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virExecWithHook; virSetNonBlock; virFormatMacAddr; virGetHostname; diff --git a/src/util.c b/src/util.c --- a/src/util.c +++ b/src/util.c @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn, const fd_set *keepfd, pid_t *retpid, int infd, int *outfd, int *errfd, - int flags) { + int flags, + virExecHook hook, + void *data) +{ pid_t pid; int null, i, openmax; int pipeout[2] = {-1,-1}; @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn, childerr != childout) close(childerr); +if (hook) +(hook)(data); + if (envp) execve(argv[0], (char **) argv, (char**)envp); else @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn, } int -virExec(virConnectPtr conn, -const char *const*argv, -const char *const*envp, -const fd_set *keepfd, -pid_t *retpid, -int infd, int *outfd, int *errfd, -int flags) { +virExecWithHook(virConnectPtr conn, +const char *const*argv, +const char *const*envp, +const fd_set *keepfd, +pid_t *retpid, +int infd, int *outfd, int *errfd, +int flags, +virExecHook hook, +void *data) +{ char *argv_str; if ((argv_str = virArgvToString(argv)) == NULL) { @@ -462,7 +471,21 @@ virExec(virConnectPtr conn, VIR_FREE(argv_str); return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, - flags); + flags, hook, data); +} + +int +virExec(virConnectPtr conn, +const char *const*argv, +const char *const*envp, +const fd_set *keepfd, +pid_t *retpid, +int infd, int *outfd, int *errfd, +int flags) +{ +re
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Jim Meyering wrote: > Daniel J Walsh wrote: > > [I removed the 1900+ lines of useless context] > >> Ok, I have added your patches and make syntax-check succeeds except it >> does not like >> >> po_check >> --- po/POTFILES.in >> +++ po/POTFILES.in >> @@ -22,8 +22,6 @@ >> src/qemu_conf.c >> src/qemu_driver.c >> src/remote_internal.c >> -src/security.c >> -src/security_selinux.c >> src/storage_backend.c >> src/storage_backend_disk.c >> src/storage_backend_fs.c >> Makefile.maint: you have changed the set of files with translatable >> diagnostics; >> apply the above patch >> >> Since these files add translations, what do I need to do to get this to >> pass? > > That check passes for me, but I'm using git. > Are you using CVS? > > If so, have you cvs-added those two files? > If you have not, that would explain it. > > Otherwise, the check (which runs a script to search > all version-controlled files) may need to be adjusted. Ok that is what I missed, I was not checking them in before running the test. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmbINsACgkQrlYvE4MpobP4FgCffugZzfPrm/it38q0wVLsJRiN T6cAn1/Vdfmhoo2UF8hbaq2c9hH9/3qP =1iaw -END PGP SIGNATURE- -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version
Daniel J Walsh wrote: > [I removed the 1900+ lines of useless context] > Ok, I have added your patches and make syntax-check succeeds except it > does not like > > po_check > --- po/POTFILES.in > +++ po/POTFILES.in > @@ -22,8 +22,6 @@ > src/qemu_conf.c > src/qemu_driver.c > src/remote_internal.c > -src/security.c > -src/security_selinux.c > src/storage_backend.c > src/storage_backend_disk.c > src/storage_backend_fs.c > Makefile.maint: you have changed the set of files with translatable > diagnostics; > apply the above patch > > Since these files add translations, what do I need to do to get this to > pass? That check passes for me, but I'm using git. Are you using CVS? If so, have you cvs-added those two files? If you have not, that would explain it. Otherwise, the check (which runs a script to search all version-controlled files) may need to be adjusted. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list