implement usb and pci hot attach in AppArmor driver

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

src/security/security_apparmor.c

@@ -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 *profile,
-             const char *fn)
+             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(virSecurityDriverPtr drv,
 
     /* 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(virSecurityDriverPtr drv,
                                   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(virSecurityDriverPtr drv,
 
         /* 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,69 @@ AppArmorReserveSecurityLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
 }
 
 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 */
+    if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
-    return 0;
+        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:
+    VIR_FREE(ptr);
+    return ret;
 }
 
+
 static int
-AppArmorRestoreSecurityHostdevLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
+AppArmorRestoreSecurityHostdevLabel(virSecurityDriverPtr drv,
                                     virDomainObjPtr vm,
                                     virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
 
@@ -646,8 +749,7 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
     if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
         return 0;
 
-    /* TODO: call load_profile (needs virDomainObjPtr vm) */
+    return reload_profile(drv, vm, NULL, false);
-    return 0;
 }
 
 static int
@@ -655,7 +757,7 @@ AppArmorSetSavedStateLabel(virSecurityDriverPtr drv,
                            virDomainObjPtr vm,
                            const char *savefile)
 {
-    return reload_profile(drv, vm, savefile);
+    return reload_profile(drv, vm, savefile, true);
 }
 
 
@@ -664,7 +766,7 @@ AppArmorRestoreSavedStateLabel(virSecurityDriverPtr drv,
                                virDomainObjPtr vm,
                                const char *savefile ATTRIBUTE_UNUSED)
 {
-    return reload_profile(drv, vm, NULL);
+    return reload_profile(drv, vm, NULL, false);
 }
 
 virSecurityDriver virAppArmorSecurityDriver = {

src/security/virt-aa-helper.c

@@ -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, const char cmd)
  * 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) {
+    if (virFileExists(include_file)) {
-        vah_error(NULL, 0, "could not allocate memory for profile");
+        flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing);
-        return rc;
+        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 clean;
+        }
+    } else {
+        if (virAsprintf(&pcontent, "%s%s", warning, included_files) == -1) {
+            vah_error(NULL, 0, "could not allocate memory for profile");
+            goto clean;
+        }
     }
 
     plen = strlen(pcontent);
@@ -248,20 +266,9 @@ update_include_file(const char *include_file, const char *included_files)
     }
 
     /* only update the disk profile if it is different */
-    if (virFileExists(include_file)) {
+    if (flen > 0 && flen == plen && STREQLEN(existing, pcontent, plen)) {
-        char *existing = NULL;
+        rc = 0;
-        int flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing);
+        goto clean;
-        if (flen < 0)
-            goto clean;
-
-        if (flen == plen) {
-            if (STREQLEN(existing, pcontent, plen)) {
-                rc = 0;
-                VIR_FREE(existing);
-                goto clean;
-            }
-        }
-        VIR_FREE(existing);
     }
 
     /* write the file */
@@ -284,6 +291,7 @@ update_include_file(const char *include_file, const char *included_files)
 
   clean:
     VIR_FREE(pcontent);
+    VIR_FREE(existing);
 
     return rc;
 }
@@ -958,8 +966,8 @@ get_files(vahControl * ctl)
             } /* switch */
         }
 
-    if (ctl->newdisk)
+    if (ctl->newfile)
-        if (vah_add_file(&buf, ctl->newdisk, "rw") != 0)
+        if (vah_add_file(&buf, ctl->newfile, "rw") != 0)
             goto clean;
 
     if (virBufferError(&buf)) {
@@ -987,6 +995,7 @@ vahParseArgv(vahControl * ctl, int argc, char **argv)
         {"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 +1003,7 @@ vahParseArgv(vahControl * ctl, int argc, char **argv)
         {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 +1019,10 @@ vahParseArgv(vahControl * ctl, int argc, char **argv)
                 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");
+                ctl->append = arg == 'F';
                 break;
             case 'h':
                 vah_usage();
@@ -1127,14 +1138,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",
+        if (ctl->append && ctl->newfile) {
-                          LOCAL_STATE_DIR, ctl->def->name);
+            if (vah_add_file(&buf, ctl->newfile, "rw") != 0)
-        virBufferVSprintf(&buf, "  \"%s/lib/libvirt/**/%s.monitor\" rw,\n",
+                goto clean;
-                          LOCAL_STATE_DIR, ctl->def->name);
+        } else {
-        virBufferVSprintf(&buf, "  \"%s/run/libvirt/**/%s.pid\" rwk,\n",
+            virBufferVSprintf(&buf, "  \"%s/log/libvirt/**/%s.log\" w,\n",
-                          LOCAL_STATE_DIR, ctl->def->name);
+                              LOCAL_STATE_DIR, ctl->def->name);
-        if (ctl->files)
+            virBufferVSprintf(&buf, "  \"%s/lib/libvirt/**/%s.monitor\" rw,\n",
-            virBufferVSprintf(&buf, "%s", ctl->files);
+                              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 +1165,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;
 
 

tests/virt-aa-helper-test

@@ -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 -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 (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"