4:15 p.m.
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
4:19 p.m.
New subject: [libvirt] [PATCH] 1/10 AppArmor driver updates
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
9:08 a.m.
New subject: [libvirt] [PATCH] 1/10 AppArmor driver updates
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:19 p.m.
New subject: [libvirt] [PATCH] 2/10 AppArmor driver updates
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
9:20 a.m.
New subject: [libvirt] [PATCH] 2/10 AppArmor driver updates
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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:20 p.m.
New subject: [libvirt] [PATCH] 3/10 AppArmor driver updates
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
9:49 a.m.
New subject: [libvirt] [PATCH] 3/10 AppArmor driver updates
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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:21 p.m.
New subject: [libvirt] [PATCH] 4/10 AppArmor driver updates
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
4:58 a.m.
New subject: [libvirt] [PATCH] 4/10 AppArmor driver updates
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 :|
4:21 p.m.
New subject: [libvirt] [PATCH] 5/10 AppArmor driver updates
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
4:59 a.m.
New subject: [libvirt] [PATCH] 5/10 AppArmor driver updates
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 :|
11:15 a.m.
New subject: [libvirt] [PATCH] 5/10 AppArmor driver updates
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,
};
1:45 p.m.
New subject: [libvirt] [PATCH] 5/10 AppArmor driver updates
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]
>
4:22 p.m.
New subject: [libvirt] [PATCH] 6/10 AppArmor driver updates
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
10:24 a.m.
New subject: [libvirt] [PATCH] 6/10 AppArmor driver updates
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:22 p.m.
New subject: [libvirt] [PATCH] 7/10 AppArmor driver updates
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
11 a.m.
New subject: [libvirt] [PATCH] 7/10 AppArmor driver updates
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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:23 p.m.
New subject: [libvirt] [PATCH] 8/10 AppArmor driver updates
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
3:55 p.m.
New subject: [libvirt] [PATCH] 8/10 AppArmor driver updates
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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:23 p.m.
New subject: [libvirt] [PATCH] 9/10 AppArmor driver updates
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
4:24 p.m.
New subject: [libvirt] [PATCH] 10/10 AppArmor driver updates
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
4:05 p.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:56 p.m.
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
10:30 a.m.
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
4:59 a.m.
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 :|
5281
days inactive
5344
days old
24 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Jamie Strandboge -
Laine Stump