[libvirt] [PATCH] 0/10AppArmor driver updates
This patch series addresses bug fixes in the AppArmor driver as well as updating it to changes made in 0.7.6 and 0.7.7. All of these are self-contained within the driver except 4_qemu_driver_stdin_path.patch. This is required by 5_apparmor-fix-save-restore.patch (see below). These all pass 'make syntax-check' and 'make check' (except 'daemon-conf', which has never passed here and I didn't patch it). 1_apparmor-dont-clear-caps.patch: originally submitted on 2010/02/08 with no feedback. The calls to virExec() in security_apparmor.c when invoking virt-aa-helper use VIR_EXEC_CLEAR_CAPS. When compiled without libcap-ng, this is not a problem (it's effectively a no-op) but with libcap-ng this causes MAC_ADMIN to be cleared. MAC_ADMIN is needed by virt-aa-helper to manipulate apparmor profiles and without it VMs will not start[1]. This patch calls virExec with the default VIR_EXEC_NONE instead. 2_apparmor-remove-unloaded-profile-is-not-fatal.patch: Don't exit with error if the user unloaded the profile outside of libvirt[2] 3_apparmor-fix-vah-xml-parse.patch: add VIR_DOMAIN_XML_INACTIVE flag to virDomainDefParseString() so virDomainDefParseString() doesn't error out when seeing <seclabel...>. This was needed due to changes since 0.7.5. 4_qemu_driver_stdin_path.patch: adjust args to qemudStartVMDaemon() to also specify path to stdin_fd, so this can be passed to the AppArmor driver via *SetSecurityAllLabel(). This updates all calls to qemudStartVMDaemon() as well as setting up the non-AppArmor security driver *SetSecurityAllLabel() declarations for the above. This is required for 5_apparmor-fix-save-restore.patch since AppArmor resolves the passed file descriptor to the pathname given to open(). 5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3] 6_apparmor-fix-backingstore.patch: adjust virt-aa-helper to handle backing store[4] 7_apparmor-fix-hostdev.patch: adjust virt-aa-helper to handle pci devices. Update valid_path() to have an override array to check against, and add "/sys/devices/pci" to it. Then rename file_iterate_cb() to file_iterate_hostdev_cb() and create file_iterate_pci_cb() based on it. 8_apparmor-fix-xauth.patch: adjust virt-aa-helper to handle SDL graphics, specifically Xauthority[6]. Also remove a couple redundant checks 9_apparmor-examples.patch: adjustments to the example profiles 10_apparmor-vah-test.patch: update pcidev test and add SDL xauth [1] https://launchpad.net/bugs/517714 [2] https://launchpad.net/bugs/530400 [3] https://launchpad.net/bugs/457716 [4] https://launchpad.net/bugs/470636 [5] https://launchpad.net/bugs/545795 [6] https://launchpad.net/bugs/545426 -- Jamie Strandboge | http://www.canonical.com
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
1_apparmor-dont-clear-caps.patch: originally submitted on 2010/02/08 with no feedback. The calls to virExec() in security_apparmor.c when invoking virt-aa-helper use VIR_EXEC_CLEAR_CAPS. When compiled without libcap-ng, this is not a problem (it's effectively a no-op) but with libcap-ng this causes MAC_ADMIN to be cleared. MAC_ADMIN is needed by virt-aa-helper to manipulate apparmor profiles and without it VMs will not start[1]. This patch calls virExec with the default VIR_EXEC_NONE instead.
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:19:03PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
1_apparmor-dont-clear-caps.patch: originally submitted on 2010/02/08 with no feedback. The calls to virExec() in security_apparmor.c when invoking virt-aa-helper use VIR_EXEC_CLEAR_CAPS. When compiled without libcap-ng, this is not a problem (it's effectively a no-op) but with libcap-ng this causes MAC_ADMIN to be cleared. MAC_ADMIN is needed by virt-aa-helper to manipulate apparmor profiles and without it VMs will not start[1]. This patch calls virExec with the default VIR_EXEC_NONE instead.
Okay, we should have reviewed this at the time, sorry. Fairly contained, so applied and commited, I will push it soon, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
2_apparmor-remove-unloaded-profile-is-not-fatal.patch: Don't exit with error if the user unloaded the profile outside of libvirt[2]
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:19:36PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
2_apparmor-remove-unloaded-profile-is-not-fatal.patch: Don't exit with error if the user unloaded the profile outside of libvirt[2]
-- Jamie Strandboge | http://www.canonical.com
Description: Don't exit with error if the user unloaded the profile outside of libvirt Author: Jamie Strandboge <jamie@canonical.com> Bug-Ubuntu: https://launchpad.net/ubuntu/bugs/530400
Okay, I think I understand the problem and checking the return value of the parser make sense in any case, applied, will push soon, thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
3_apparmor-fix-vah-xml-parse.patch: add VIR_DOMAIN_XML_INACTIVE flag to virDomainDefParseString() so virDomainDefParseString() doesn't error out when seeing <seclabel...>. This was needed due to changes since 0.7.5.
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:20:04PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
3_apparmor-fix-vah-xml-parse.patch: add VIR_DOMAIN_XML_INACTIVE flag to virDomainDefParseString() so virDomainDefParseString() doesn't error out when seeing <seclabel...>. This was needed due to changes since 0.7.5.
-- Jamie Strandboge | http://www.canonical.com
Author: Jamie Strandboge <jamie@ubuntu.com> Description: add VIR_DOMAIN_XML_INACTIVE flag to virDomainDefParseString() so virDomainDefParseString() doesn't error out when seeing <seclabel...>
Okay, fairly contained too, applied, thanks Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
4_qemu_driver_stdin_path.patch: adjust args to qemudStartVMDaemon() to also specify path to stdin_fd, so this can be passed to the AppArmor driver via *SetSecurityAllLabel(). This updates all calls to qemudStartVMDaemon() as well as setting up the non-AppArmor security driver *SetSecurityAllLabel() declarations for the above. This is required for 5_apparmor-fix-save-restore.patch since AppArmor resolves the passed file descriptor to the pathname given to open().
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:21:16PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
4_qemu_driver_stdin_path.patch: adjust args to qemudStartVMDaemon() to also specify path to stdin_fd, so this can be passed to the AppArmor driver via *SetSecurityAllLabel(). This updates all calls to qemudStartVMDaemon() as well as setting up the non-AppArmor security driver *SetSecurityAllLabel() declarations for the above. This is required for 5_apparmor-fix-save-restore.patch since AppArmor resolves the passed file descriptor to the pathname given to open().
ACK, this is somewhat gross, but I don't see an immediate alternative option. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:21:46PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Jamie, Likewise, this patch was ACKed but not pushed. Still okay to push? On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
-- Jamie Strandboge | http://www.canonical.com Author: Jamie Strandboge <jamie canonical com> Description: update AppArmor security driver to adjust profile for save/restore Bug: https://bugzilla.redhat.com/show_bug.cgi?id=529363 Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/457716 Index: libvirt-0.7.7/src/security/security_apparmor.c =================================================================== --- libvirt-0.7.7.orig/src/security/security_apparmor.c 2010-03-31 11:20:48.000000000 -0500 +++ libvirt-0.7.7/src/security/security_apparmor.c 2010-03-31 11:31:39.000000000 -0500 @@ -149,7 +149,7 @@ */ static int load_profile(const char *profile, virDomainObjPtr vm, - virDomainDiskDefPtr disk) + const char *fn) { int rc = -1, status, ret; bool create = true; @@ -175,9 +175,9 @@ }; ret = virExec(argv, NULL, NULL, &child, pipefd[0], NULL, NULL, VIR_EXEC_NONE); - } else if (disk && disk->src) { + } else if (fn) { const char *const argv[] = { - VIRT_AA_HELPER, "-r", "-u", profile, "-f", disk->src, NULL + VIRT_AA_HELPER, "-r", "-u", profile, "-f", fn, NULL }; ret = virExec(argv, NULL, NULL, &child, pipefd[0], NULL, NULL, VIR_EXEC_NONE); @@ -277,6 +277,40 @@ return rc; } +/* reload the profile, adding read/write file specified by fn if it is not + * NULL. + */ +static int +reload_profile(virDomainObjPtr vm, const char *fn) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + int rc = -1; + char *profile_name = NULL; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + if ((profile_name = get_profile_name(vm)) == NULL) + return rc; + + /* Update the profile only if it is loaded */ + if (profile_loaded(secdef->imagelabel) >= 0) { + if (load_profile(secdef->imagelabel, vm, fn) < 0) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot update AppArmor profile " + "\'%s\'"), + secdef->imagelabel); + goto clean; + } + } + + rc = 0; + clean: + VIR_FREE(profile_name); + + return rc; +} + /* Called on libvirtd startup to see if AppArmor is available */ static int AppArmorSecurityDriverProbe(void) @@ -377,14 +411,14 @@ } static int -AppArmorSetSecurityAllLabel(virDomainObjPtr vm) +AppArmorSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path) { if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) return 0; /* if the profile is not already loaded, then load one */ if (profile_loaded(vm->def->seclabel.label) < 0) { - if (load_profile(vm->def->seclabel.label, vm, NULL) < 0) { + if (load_profile(vm->def->seclabel.label, vm, stdin_path) < 0) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("cannot generate AppArmor profile " "\'%s\'"), vm->def->seclabel.label); @@ -501,32 +535,7 @@ AppArmorRestoreSecurityImageLabel(virDomainObjPtr vm, virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) { - const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - int rc = -1; - char *profile_name = NULL; - - if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) - return 0; - - if ((profile_name = get_profile_name(vm)) == NULL) - return rc; - - /* Update the profile only if it is loaded */ - if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(secdef->imagelabel, vm, NULL) < 0) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot update AppArmor profile " - "\'%s\'"), - secdef->imagelabel); - goto clean; - } - } - - rc = 0; - clean: - VIR_FREE(profile_name); - - return rc; + return reload_profile(vm, NULL); } /* Called when hotplugging */ @@ -556,7 +565,7 @@ /* update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(secdef->imagelabel, vm, disk) < 0) { + if (load_profile(secdef->imagelabel, vm, disk->src) < 0) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("cannot update AppArmor profile " "\'%s\'"), @@ -623,6 +632,21 @@ return 0; } +static int +AppArmorSetSavedStateLabel(virDomainObjPtr vm, + const char *savefile) +{ + return reload_profile(vm, savefile); +} + + +static int +AppArmorRestoreSavedStateLabel(virDomainObjPtr vm, + const char *savefile ATTRIBUTE_UNUSED) +{ + return reload_profile(vm, NULL); +} + virSecurityDriver virAppArmorSecurityDriver = { .name = SECURITY_APPARMOR_NAME, .probe = AppArmorSecurityDriverProbe, @@ -639,4 +663,6 @@ .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel, .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel, + .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, + .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel, };
On 06/04/2010 12:15 PM, Laine Stump wrote:
Jamie,
Likewise, this patch was ACKed but not pushed. Still okay to push?
I also pushed this patch, after getting a response from Jamie.
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
6_apparmor-fix-backingstore.patch: adjust virt-aa-helper to handle backing store[4]
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:22:10PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
6_apparmor-fix-backingstore.patch: adjust virt-aa-helper to handle backing store[4]
-- Jamie Strandboge | http://www.canonical.com
Also self contained and easy to understand, applied ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
7_apparmor-fix-hostdev.patch: adjust virt-aa-helper to handle pci devices. Update valid_path() to have an override array to check against, and add "/sys/devices/pci" to it. Then rename file_iterate_cb() to file_iterate_hostdev_cb() and create file_iterate_pci_cb() based on it.
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:22:43PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
7_apparmor-fix-hostdev.patch: adjust virt-aa-helper to handle pci devices. Update valid_path() to have an override array to check against, and add "/sys/devices/pci" to it. Then rename file_iterate_cb() to file_iterate_hostdev_cb() and create file_iterate_pci_cb() based on it.
-- Jamie Strandboge | http://www.canonical.com
Author: Jamie Strandboge <jamie@canonical.com> Description: adjust virt-aa-helper to handle pci devices. Update valid_path() to have an override array to check against, and add "/sys/devices/pci" to it. Then rename file_iterate_cb() to file_iterate_hostdev_cb() and create file_iterate_pci_cb() based on it. Bug-Ubuntu: https://launchpad.net/bugs/545795
Okay, that looks like an important update, small and well contained, so applied too, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
8_apparmor-fix-xauth.patch: adjust virt-aa-helper to handle SDL graphics, specifically Xauthority[6]. Also remove a couple redundant checks
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:23:11PM -0500, Jamie Strandboge wrote:
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
8_apparmor-fix-xauth.patch: adjust virt-aa-helper to handle SDL graphics, specifically Xauthority[6]. Also remove a couple redundant checks
-- Jamie Strandboge | http://www.canonical.com
Author: Jamie Strandboge <jamie@canonical.com> Description: adjust virt-aa-helper to handle SDL graphics, specifically Xauthority. Also remove a couple redundant checks. Bug-Ubuntu: https://launchpad.net/bugs/545426
Looking safe and 3 nice cleanups, commited ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
9_apparmor-examples.patch: adjustments to the example profiles
-- Jamie Strandboge | http://www.canonical.com
On Mon, 2010-04-05 at 16:15 -0500, Jamie Strandboge wrote:
10_apparmor-vah-test.patch: update pcidev test and add SDL xauth
-- Jamie Strandboge | http://www.canonical.com
On Mon, Apr 05, 2010 at 04:15:06PM -0500, Jamie Strandboge wrote:
This patch series addresses bug fixes in the AppArmor driver as well as updating it to changes made in 0.7.6 and 0.7.7. All of these are self-contained within the driver except 4_qemu_driver_stdin_path.patch. This is required by 5_apparmor-fix-save-restore.patch (see below). These all pass 'make syntax-check' and 'make check' (except 'daemon-conf', which has never passed here and I didn't patch it). [...]
4_qemu_driver_stdin_path.patch: adjust args to qemudStartVMDaemon() to also specify path to stdin_fd, so this can be passed to the AppArmor driver via *SetSecurityAllLabel(). This updates all calls to qemudStartVMDaemon() as well as setting up the non-AppArmor security driver *SetSecurityAllLabel() declarations for the above. This is required for 5_apparmor-fix-save-restore.patch since AppArmor resolves the passed file descriptor to the pathname given to open().
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
Okay, I have now pushed all the patches except 4/ and 5/ which were changing some of the internal secutity layers of the qemud and it sounds a bit late in the cycle for this, plus I would prefer some review from Dan before pushing like this, but 1-3, 7-10 are in :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, 2010-04-06 at 23:05 +0200, Daniel Veillard wrote:
On Mon, Apr 05, 2010 at 04:15:06PM -0500, Jamie Strandboge wrote:
This patch series addresses bug fixes in the AppArmor driver as well as updating it to changes made in 0.7.6 and 0.7.7. All of these are self-contained within the driver except 4_qemu_driver_stdin_path.patch. This is required by 5_apparmor-fix-save-restore.patch (see below). These all pass 'make syntax-check' and 'make check' (except 'daemon-conf', which has never passed here and I didn't patch it). [...]
4_qemu_driver_stdin_path.patch: adjust args to qemudStartVMDaemon() to also specify path to stdin_fd, so this can be passed to the AppArmor driver via *SetSecurityAllLabel(). This updates all calls to qemudStartVMDaemon() as well as setting up the non-AppArmor security driver *SetSecurityAllLabel() declarations for the above. This is required for 5_apparmor-fix-save-restore.patch since AppArmor resolves the passed file descriptor to the pathname given to open().
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
Okay, I have now pushed all the patches except 4/ and 5/ which were changing some of the internal secutity layers of the qemud and it sounds a bit late in the cycle for this, plus I would prefer some review from Dan before pushing like this,
Just for a little more background on '4': qemudStartVMDaemon() passes stdin_fd to virExecDaemonize(), which launches qemu passing it the file descriptor if it is not -1. qemudDomainRestore() is the only place where stdin_fd is a valid file descriptor (not -1). However, when qemu is launched with this fd, the kernel resolves this to the real path and checks to see if the process has access to this path, and the restore fails because this path is not in the AppArmor profile. Since there is no easy/portable way to obtain a pathname based on a file descriptor, we need to pass the path name given to open() (when we got the stdin_fd) to qemudStartVMDaemon() so that we can then turn around and give it to driver->securityDriver->domainSetSecurityAllLabel(). This way the AppArmor security driver can update the profile using the pathname. In my patch, qemudStartVMDaemon() is updated to have a new argument: stdin_path. All calls to qemudStartVMDaemon() add NULL for this added argument, except the one call in qemudDomainRestore(), which uses 'path' (the pathname given to open() to obtain 'fd'). qemudStartVMDaemon() is updated to pass stdin_path to domainSetSecurityAllLabel(). security_driver.h, qemu_security_dac.c, qemu_security_stacked.c and security_selinux.c are adjusted to have the extra argument in the *Set*AllLabel functions, with the drivers all using ATTRIBUTE_UNUSED. '5' adjusts the apparmor driver to implement domainSetSavedStateLabel and domainRestoreSavedStateLabel based on the above, and also has some refactoring for code reuse by these functions.
but 1-3, 7-10 are in :-)
Actually, it is 1-3 and 6-10 that were committed. Thanks for the review and commits! :) -- Jamie Strandboge | http://www.canonical.com
Top posting since this is only a reminder... I was wondering if this could be re-reviewed now that 0.8 is out. Thanks! Jamie On Tue, 2010-04-06 at 16:56 -0500, Jamie Strandboge wrote:
On Tue, 2010-04-06 at 23:05 +0200, Daniel Veillard wrote:
On Mon, Apr 05, 2010 at 04:15:06PM -0500, Jamie Strandboge wrote:
This patch series addresses bug fixes in the AppArmor driver as well as updating it to changes made in 0.7.6 and 0.7.7. All of these are self-contained within the driver except 4_qemu_driver_stdin_path.patch. This is required by 5_apparmor-fix-save-restore.patch (see below). These all pass 'make syntax-check' and 'make check' (except 'daemon-conf', which has never passed here and I didn't patch it). [...]
4_qemu_driver_stdin_path.patch: adjust args to qemudStartVMDaemon() to also specify path to stdin_fd, so this can be passed to the AppArmor driver via *SetSecurityAllLabel(). This updates all calls to qemudStartVMDaemon() as well as setting up the non-AppArmor security driver *SetSecurityAllLabel() declarations for the above. This is required for 5_apparmor-fix-save-restore.patch since AppArmor resolves the passed file descriptor to the pathname given to open().
5_apparmor-fix-save-restore.patch: refactoring to update AppArmor security driver to adjust profile for save/restore[3]
Okay, I have now pushed all the patches except 4/ and 5/ which were changing some of the internal secutity layers of the qemud and it sounds a bit late in the cycle for this, plus I would prefer some review from Dan before pushing like this,
Just for a little more background on '4': qemudStartVMDaemon() passes stdin_fd to virExecDaemonize(), which launches qemu passing it the file descriptor if it is not -1. qemudDomainRestore() is the only place where stdin_fd is a valid file descriptor (not -1). However, when qemu is launched with this fd, the kernel resolves this to the real path and checks to see if the process has access to this path, and the restore fails because this path is not in the AppArmor profile. Since there is no easy/portable way to obtain a pathname based on a file descriptor, we need to pass the path name given to open() (when we got the stdin_fd) to qemudStartVMDaemon() so that we can then turn around and give it to driver->securityDriver->domainSetSecurityAllLabel(). This way the AppArmor security driver can update the profile using the pathname.
In my patch, qemudStartVMDaemon() is updated to have a new argument: stdin_path. All calls to qemudStartVMDaemon() add NULL for this added argument, except the one call in qemudDomainRestore(), which uses 'path' (the pathname given to open() to obtain 'fd'). qemudStartVMDaemon() is updated to pass stdin_path to domainSetSecurityAllLabel(). security_driver.h, qemu_security_dac.c, qemu_security_stacked.c and security_selinux.c are adjusted to have the extra argument in the *Set*AllLabel functions, with the drivers all using ATTRIBUTE_UNUSED.
'5' adjusts the apparmor driver to implement domainSetSavedStateLabel and domainRestoreSavedStateLabel based on the above, and also has some refactoring for code reuse by these functions.
but 1-3, 7-10 are in :-)
Actually, it is 1-3 and 6-10 that were committed. Thanks for the review and commits! :)
-- Jamie Strandboge | http://www.canonical.com
On Tue, Apr 13, 2010 at 10:30:22AM -0500, Jamie Strandboge wrote:
Top posting since this is only a reminder...
I was wondering if this could be re-reviewed now that 0.8 is out.
Sorry for not doing this before - I saw DV had pushed the patches previously, but didn't realize he left out 2 for more review. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Jamie Strandboge -
Laine Stump