Re: [libvirt] [PATCH] 0/10AppArmor driver updates

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! :)