The AppArmor security driver has partial support for hostdev devices in that if they already exist in the XML, virt-aa-helper can find them and add them to the profile. Hot attach does not work[1] because AppArmorSetSecurityHostdevLabel and AppArmorRestoreSecurityHostdevLabel are not currently implemented. From the patch description: Implement AppArmorSetSecurityHostdevLabel() and AppArmorRestoreSecurityHostdevLabel() for hostdev and pcidev attach. virt-aa-helper also has to be adjusted because *FileIterate() is used for pci and usb devices and the corresponding XML for hot attached hostdev and pcidev is not in the XML passed to virt-aa-helper. The new '-F filename' option is added to append a rule to the profile as opposed to the existing '-f filename', which rewrites the libvirt-<uuid>.files file anew. This new '-F' option will append a rule to an existing libvirt-<uuid>.files if it exists, otherwise it acts the same as '-f'. load_profile() and reload_profile() have been adjusted to add an 'append' argument, which when true will use '-F' instead of '-f' when executing virt-aa-helper. All existing calls to load_profile() and reload_profile() have been adjusted to use the old behavior (ie append==false) except AppArmorSetSavedStateLabel() where it made sense to use the new behavior. This patch also adds tests for '-F' The tests still use the old convention of cat with sed that Eric Blake mentioned should be improved-- I will be submitting another patch for this. This patch compiles fine with --enable-compile-warnings=error, passes the parts of 'make check' that this patch touches (ie, the daemon-conf fails here, but it always fails for me) and passes 'syntax-check'. Jamie [1]https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/640993 -- Jamie Strandboge | http://www.canonical.com
Author: Jamie Strandboge <jamie@xxxxxxxxxxxxx> Description: Implement AppArmorSetSecurityHostdevLabel() and AppArmorRestoreSecurityHostdevLabel() for hostdev and pcidev attach. virt-aa-helper also has to be adjusted because *FileIterate() is used for pci and usb devices and the corresponding XML for hot attached hostdev and pcidev is not in the XML passed to virt-aa-helper. The new '-F filename' option is added to append a rule to the profile as opposed to the existing '-f filename', which rewrites the libvirt-<uuid>.files file anew. This new '-F' option will append a rule to an existing libvirt-<uuid>.files if it exists, otherwise it acts the same as '-f'. load_profile() and reload_profile() have been adjusted to add an 'append' argument, which when true will use '-F' instead of '-f' when executing virt-aa-helper. All existing calls to load_profile() and reload_profile() have been adjusted to use the old behavior (ie append==false) except AppArmorSetSavedStateLabel() where it made sense to use the new behavior. This patch also adds tests for '-F'. Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/640993 diff -Naurp libvirt.orig/src/security/security_apparmor.c libvirt/src/security/security_apparmor.c --- libvirt.orig/src/security/security_apparmor.c 2010-09-22 16:07:21.000000000 -0500 +++ libvirt/src/security/security_apparmor.c 2010-09-23 16:36:09.000000000 -0500 @@ -1,7 +1,7 @@ /* * AppArmor security driver for libvirt - * Copyright (C) 2009 Canonical Ltd. + * Copyright (C) 2009-2010 Canonical Ltd. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -35,12 +35,20 @@ #include "virterror_internal.h" #include "datatypes.h" #include "uuid.h" +#include "pci.h" +#include "hostusb.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_APPARMOR_VOID_DOI "0" #define SECURITY_APPARMOR_NAME "apparmor" #define VIRT_AA_HELPER BINDIR "/virt-aa-helper" +/* Data structure to pass to *FileIterate so we have everything we need */ +struct SDPDOP { + virSecurityDriverPtr drv; + virDomainObjPtr vm; +}; + /* * profile_status returns '-1' on error, '0' if loaded * @@ -149,8 +157,10 @@ profile_status_file(const char *str) */ static int load_profile(virSecurityDriverPtr drv, - const char *profile, virDomainObjPtr vm, - const char *fn) + const char *profile, + virDomainObjPtr vm, + const char *fn, + bool append) { int rc = -1, status, ret; bool create = true; @@ -178,6 +188,12 @@ load_profile(virSecurityDriverPtr drv, }; ret = virExec(argv, NULL, NULL, &child, pipefd[0], NULL, NULL, VIR_EXEC_NONE); + } else if (fn && append) { + const char *const argv[] = { + VIRT_AA_HELPER, "-p", probe, "-r", "-u", profile, "-F", fn, NULL + }; + ret = virExec(argv, NULL, NULL, &child, + pipefd[0], NULL, NULL, VIR_EXEC_NONE); } else if (fn) { const char *const argv[] = { VIRT_AA_HELPER, "-p", probe, "-r", "-u", profile, "-f", fn, NULL @@ -285,7 +301,9 @@ cleanup: */ static int reload_profile(virSecurityDriverPtr drv, - virDomainObjPtr vm, const char *fn) + virDomainObjPtr vm, + const char *fn, + bool append) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int rc = -1; @@ -299,7 +317,7 @@ reload_profile(virSecurityDriverPtr drv, /* Update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(drv, secdef->imagelabel, vm, fn) < 0) { + if (load_profile(drv, secdef->imagelabel, vm, fn, append) < 0) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("cannot update AppArmor profile " "\'%s\'"), @@ -315,6 +333,42 @@ reload_profile(virSecurityDriverPtr drv, return rc; } +static int +AppArmorSetSecurityUSBLabel(usbDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ + struct SDPDOP *ptr = opaque; + virDomainObjPtr vm = ptr->vm; + + if (reload_profile(ptr->drv, vm, file, true) < 0) { + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot update AppArmor profile " + "\'%s\'"), + secdef->imagelabel); + return -1; + } + return 0; +} + +static int +AppArmorSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ + struct SDPDOP *ptr = opaque; + virDomainObjPtr vm = ptr->vm; + + if (reload_profile(ptr->drv, vm, file, true) < 0) { + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot update AppArmor profile " + "\'%s\'"), + secdef->imagelabel); + return -1; + } + return 0; +} + /* Called on libvirtd startup to see if AppArmor is available */ static int AppArmorSecurityDriverProbe(void) @@ -426,7 +480,8 @@ AppArmorSetSecurityAllLabel(virSecurityD /* if the profile is not already loaded, then load one */ if (profile_loaded(vm->def->seclabel.label) < 0) { - if (load_profile(drv, vm->def->seclabel.label, vm, stdin_path) < 0) { + if (load_profile(drv, vm->def->seclabel.label, vm, stdin_path, + false) < 0) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("cannot generate AppArmor profile " "\'%s\'"), vm->def->seclabel.label); @@ -549,7 +604,7 @@ AppArmorRestoreSecurityImageLabel(virSec virDomainObjPtr vm, virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) { - return reload_profile(drv, vm, NULL); + return reload_profile(drv, vm, NULL, false); } /* Called when hotplugging */ @@ -580,7 +635,8 @@ AppArmorSetSecurityImageLabel(virSecurit /* update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(drv, secdef->imagelabel, vm, disk->src) < 0) { + if (load_profile(drv, secdef->imagelabel, vm, disk->src, + false) < 0) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("cannot update AppArmor profile " "\'%s\'"), @@ -622,22 +678,71 @@ AppArmorReserveSecurityLabel(virSecurity } static int -AppArmorSetSecurityHostdevLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, +AppArmorSetSecurityHostdevLabel(virSecurityDriverPtr drv, virDomainObjPtr vm, - virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED) + virDomainHostdevDefPtr dev) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + struct SDPDOP *ptr; + int ret = -1; if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) return 0; - /* TODO: call load_profile with an update vm->def */ - return 0; + if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return 0; + + if (profile_loaded(secdef->imagelabel) < 0) + return 0; + + if (VIR_ALLOC(ptr) < 0) + return -1; + ptr->drv = drv; + ptr->vm = vm; + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + usbDevice *usb = usbGetDevice(dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + + if (!usb) + goto done; + + ret = usbDeviceFileIterate(usb, AppArmorSetSecurityUSBLabel, ptr); + usbFreeDevice(usb); + break; + } + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + pciDevice *pci = pciGetDevice(dev->source.subsys.u.pci.domain, + dev->source.subsys.u.pci.bus, + dev->source.subsys.u.pci.slot, + dev->source.subsys.u.pci.function); + + if (!pci) + goto done; + + ret = pciDeviceFileIterate(pci, AppArmorSetSecurityPCILabel, ptr); + pciFreeDevice(pci); + break; + } + + default: + ret = 0; + break; + } + +done: + ptr->drv = NULL; + ptr->vm = NULL; + VIR_FREE(ptr); + return ret; } + static int -AppArmorRestoreSecurityHostdevLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, +AppArmorRestoreSecurityHostdevLabel(virSecurityDriverPtr drv, virDomainObjPtr vm, virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED) @@ -646,8 +751,7 @@ AppArmorRestoreSecurityHostdevLabel(virS if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) return 0; - /* TODO: call load_profile (needs virDomainObjPtr vm) */ - return 0; + return reload_profile(drv, vm, NULL, false); } static int @@ -655,7 +759,7 @@ AppArmorSetSavedStateLabel(virSecurityDr virDomainObjPtr vm, const char *savefile) { - return reload_profile(drv, vm, savefile); + return reload_profile(drv, vm, savefile, true); } @@ -664,7 +768,7 @@ AppArmorRestoreSavedStateLabel(virSecuri virDomainObjPtr vm, const char *savefile ATTRIBUTE_UNUSED) { - return reload_profile(drv, vm, NULL); + return reload_profile(drv, vm, NULL, false); } virSecurityDriver virAppArmorSecurityDriver = { diff -Naurp libvirt.orig/src/security/virt-aa-helper.c libvirt/src/security/virt-aa-helper.c --- libvirt.orig/src/security/virt-aa-helper.c 2010-09-23 15:59:17.000000000 -0500 +++ libvirt/src/security/virt-aa-helper.c 2010-09-23 16:36:09.000000000 -0500 @@ -1,7 +1,7 @@ /* * virt-aa-helper: wrapper program used by AppArmor security driver. - * Copyright (C) 2009 Canonical Ltd. + * Copyright (C) 2009-2010 Canonical Ltd. * * See COPYING.LIB for the License of this software * @@ -54,7 +54,8 @@ typedef struct { char *hvm; /* type of hypervisor (eg hvm, xen) */ char *arch; /* machine architecture */ int bits; /* bits in the guest */ - char *newdisk; /* newly added disk */ + char *newfile; /* newly added file */ + bool append; /* append to .files instead of rewrite */ } vahControl; static int @@ -68,7 +69,7 @@ vahDeinit(vahControl * ctl) VIR_FREE(ctl->files); VIR_FREE(ctl->hvm); VIR_FREE(ctl->arch); - VIR_FREE(ctl->newdisk); + VIR_FREE(ctl->newfile); return 0; } @@ -84,6 +85,8 @@ vah_usage(void) " -a | --add load profile\n" " -c | --create create profile from template\n" " -D | --delete unload and delete profile\n" + " -f | --add-file <file> add file to profile\n" + " -F | --append-file <file> append file to profile\n" " -r | --replace reload profile\n" " -R | --remove unload profile\n" " -h | --help this help\n" @@ -227,18 +230,33 @@ parserCommand(const char *profile_name, * Update the dynamic files */ static int -update_include_file(const char *include_file, const char *included_files) +update_include_file(const char *include_file, const char *included_files, + bool append) { int rc = -1; - int plen; + int plen, flen; int fd; char *pcontent = NULL; + char *existing = NULL; const char *warning = "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n"; - if (virAsprintf(&pcontent, "%s%s", warning, included_files) == -1) { - vah_error(NULL, 0, "could not allocate memory for profile"); - return rc; + if (virFileExists(include_file)) { + flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing); + if (flen < 0) + return rc; + } + + if (append && virFileExists(include_file)) { + if (virAsprintf(&pcontent, "%s%s", existing, included_files) == -1) { + vah_error(NULL, 0, "could not allocate memory for profile"); + goto clean2; + } + } else { + if (virAsprintf(&pcontent, "%s%s", warning, included_files) == -1) { + vah_error(NULL, 0, "could not allocate memory for profile"); + goto clean2; + } } plen = strlen(pcontent); @@ -248,20 +266,9 @@ update_include_file(const char *include_ } /* only update the disk profile if it is different */ - if (virFileExists(include_file)) { - char *existing = NULL; - int flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing); - if (flen < 0) - goto clean; - - if (flen == plen) { - if (STREQLEN(existing, pcontent, plen)) { - rc = 0; - VIR_FREE(existing); - goto clean; - } - } - VIR_FREE(existing); + if (flen > 0 && flen == plen && STREQLEN(existing, pcontent, plen)) { + rc = 0; + goto clean; } /* write the file */ @@ -284,6 +291,8 @@ update_include_file(const char *include_ clean: VIR_FREE(pcontent); + clean2: + VIR_FREE(existing); return rc; } @@ -958,8 +967,8 @@ get_files(vahControl * ctl) } /* switch */ } - if (ctl->newdisk) - if (vah_add_file(&buf, ctl->newdisk, "rw") != 0) + if (ctl->newfile) + if (vah_add_file(&buf, ctl->newfile, "rw") != 0) goto clean; if (virBufferError(&buf)) { @@ -987,6 +996,7 @@ vahParseArgv(vahControl * ctl, int argc, {"dryrun", 0, 0, 'd'}, {"delete", 0, 0, 'D'}, {"add-file", 0, 0, 'f'}, + {"append-file", 0, 0, 'F'}, {"help", 0, 0, 'h'}, {"replace", 0, 0, 'r'}, {"remove", 0, 0, 'R'}, @@ -994,7 +1004,7 @@ vahParseArgv(vahControl * ctl, int argc, {0, 0, 0, 0} }; - while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:", opt, + while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:F:", opt, &idx)) != -1) { switch (arg) { case 'a': @@ -1010,8 +1020,13 @@ vahParseArgv(vahControl * ctl, int argc, ctl->cmd = 'D'; break; case 'f': - if ((ctl->newdisk = strdup(optarg)) == NULL) + case 'F': + if ((ctl->newfile = strdup(optarg)) == NULL) vah_error(ctl, 1, "could not allocate memory for disk"); + if (arg == 'F') + ctl->append = true; + else + ctl->append = false; break; case 'h': vah_usage(); @@ -1127,14 +1142,19 @@ main(int argc, char **argv) if (ctl->cmd == 'c' && virFileExists(profile)) vah_error(ctl, 1, "profile exists"); - virBufferVSprintf(&buf, " \"%s/log/libvirt/**/%s.log\" w,\n", - LOCAL_STATE_DIR, ctl->def->name); - virBufferVSprintf(&buf, " \"%s/lib/libvirt/**/%s.monitor\" rw,\n", - LOCAL_STATE_DIR, ctl->def->name); - virBufferVSprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", - LOCAL_STATE_DIR, ctl->def->name); - if (ctl->files) - virBufferVSprintf(&buf, "%s", ctl->files); + if (ctl->append && ctl->newfile) { + if (vah_add_file(&buf, ctl->newfile, "rw") != 0) + goto clean; + } else { + virBufferVSprintf(&buf, " \"%s/log/libvirt/**/%s.log\" w,\n", + LOCAL_STATE_DIR, ctl->def->name); + virBufferVSprintf(&buf, " \"%s/lib/libvirt/**/%s.monitor\" rw,\n", + LOCAL_STATE_DIR, ctl->def->name); + virBufferVSprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", + LOCAL_STATE_DIR, ctl->def->name); + if (ctl->files) + virBufferVSprintf(&buf, "%s", ctl->files); + } if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -1149,7 +1169,8 @@ main(int argc, char **argv) vah_info(included_files); rc = 0; } else if ((rc = update_include_file(include_file, - included_files)) != 0) + included_files, + ctl->append)) != 0) goto clean; diff -Naurp libvirt.orig/tests/virt-aa-helper-test libvirt/tests/virt-aa-helper-test --- libvirt.orig/tests/virt-aa-helper-test 2010-09-23 15:59:17.000000000 -0500 +++ libvirt/tests/virt-aa-helper-test 2010-09-23 16:36:09.000000000 -0500 @@ -179,6 +179,8 @@ else cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,/boot/initrd,g" > "$test_xml" testme "1" "-r with invalid -f with probing" "-p 1 -r -u $valid_uuid -f $bad_disk" "$test_xml" testme "1" "-r with invalid -f without probing" "-p 0 -r -u $valid_uuid -f $bad_disk" "$test_xml" + testme "1" "-r with invalid -F with probing" "-p 1 -r -u $valid_uuid -F $bad_disk" "$test_xml" + testme "1" "-r with invalid -F without probing" "-p 0 -r -u $valid_uuid -F $bad_disk" "$test_xml" fi cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1</disk>,g" > "$test_xml" @@ -237,6 +239,12 @@ testme "0" "replace (adding disk)" "-r - cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" > "$test_xml" testme "0" "replace (adding non-existent disk)" "-r -u $valid_uuid -f $nonexistent" "$test_xml" +cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" > "$test_xml" +testme "0" "replace (appending disk)" "-r -u $valid_uuid -F $disk2" "$test_xml" + +cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" > "$test_xml" +testme "0" "replace (appending non-existent disk)" "-r -u $valid_uuid -F $nonexistent" "$test_xml" + cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<disk type='block' device='cdrom'><target dev='hdc' bus='ide'/><readonly/></disk></devices>,g" > "$test_xml" testme "0" "disk (empty cdrom)" "-r -u $valid_uuid" "$test_xml"
Attachment:
signature.asc
Description: This is a digitally signed message part
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list