On Fri, Sep 25, 2009 at 05:49:01PM -0500, Jamie Strandboge wrote:

diff -Naurp libvirt.orig/src/security/security_apparmor.c libvirt/src/security/security_apparmor.c --- libvirt.orig/src/security/security_apparmor.c 1969-12-31 18:00:00.000000000 -0600 +++ libvirt/src/security/security_apparmor.c 2009-09-25 17:00:13.000000000 -0500 +/* + * profile_status returns '-1' on error, '0' if loaded + * + * If check_enforcing is set to '1', then returns '-1' on error, '0' if + * loaded in complain mode, and '1' if loaded in enforcing mode. + */ +static int +profile_status(const char *str, const int check_enforcing) +{ + char *content = NULL; + char *tmp = NULL; + char *etmp = NULL; + int rc = -1; + + /* create string that is '<str> \0' for accurate matching */ + if (virAsprintf(&tmp, "%s ", str) == -1) + return rc; + + if (check_enforcing != 0) { + /* create string that is '<str> (enforce)\0' for accurate matching */ + if (virAsprintf(&etmp, "%s (enforce)", str) == -1) { + VIR_FREE(tmp); + return rc; + } + } + + if (virFileReadAll(APPARMOR_PROFILES_PATH, MAX_FILE_LEN, &content) < 0) { + virReportSystemError(NULL, errno, + _("Failed to read AppArmor profiles list " + "\'%s\'"), APPARMOR_PROFILES_PATH); + if (check_enforcing != 0) + VIR_FREE(etmp); + VIR_FREE(tmp); + return rc; + } + + if (strstr(content, tmp) != NULL) + rc = 0; + if (check_enforcing != 0) { + if (rc == 0 && strstr(content, etmp) != NULL) + rc = 1; /* return '1' if loaded and enforcing */ + VIR_FREE(etmp); + }

Since you've already got yourself a 'rc' variable declared & initialized, it'd benice to replace the duplicated error paths: VIR_FREE(tmp) return rc; With just a 'goto cleanup', and put a cleanup: label just at the endd here:

+ VIR_FREE(tmp); + VIR_FREE(content); + + return rc; +} +

+/* + * profile_status_file returns '-1' on error, '0' if file on disk is in + * complain mode and '1' if file on disk is in enforcing mode + */ +static int +profile_status_file(const char *str) +{ + char profile[PATH_MAX]; + char *content = NULL; + char *tmp = NULL; + int rc = -1; + int len; + + if (snprintf(profile, PATH_MAX, "%s/%s", APPARMOR_DIR "/libvirt", str) + > PATH_MAX - 1) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + "%s", _("profile name exceeds maximum length")); + } + + if (!virFileExists(profile)) { + return rc; + } + + if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { + virReportSystemError(NULL, errno, + _("Failed to read \'%s\'"), profile); + return rc; + } + + /* create string that is ' <str> flags=(complain)\0' */ + if (virAsprintf(&tmp, " %s flags=(complain)", str) == -1) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + "%s", _("could not allocate memory"));

This one should just be virReportOOMError(NULL);

+ VIR_FREE(content); + return rc; + } + + if (strstr(content, tmp) != NULL) + rc = 0; + else + rc = 1; + + VIR_FREE(tmp); + VIR_FREE(content); + + return rc;

Similar thought here about removing the duplicated cleanup paths in favour of just jumping to the end of the method & having a single cleanup code block

+static int +load_profile(virConnectPtr conn, const char *profile, virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + const char *argv[7]; + int rc = -1, status; + bool create = true; + char *xml = NULL; + int pipefd[2]; + pid_t child; + + if (pipe(pipefd) < -1) { + virReportSystemError(conn, errno, "%s", _("unable to create pipe")); + return rc; + } + + xml = virDomainDefFormat(conn, vm->def, VIR_DOMAIN_XML_SECURE); + if (!xml) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", _("could not format XML")); + goto failed; + }

No need to report an error when the virDomainDef* functions fail, since they'll have already done that for you.

+ + if (profile_status_file(profile) >= 0) + create = false; + + argv[0] = VIRT_AA_HELPER_PATH; + argv[2] = (char *) "-u"; + argv[3] = profile; + argv[4] = NULL; + if (create) + argv[1] = (char *) "-c"; + else { + argv[1] = (char *) "-r"; + if (disk && disk->src) { + argv[4] = "-f"; + argv[5] = disk->src; + argv[6] = NULL; + } + }

+ if (virExec(conn, argv, NULL, NULL, &child, + pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS) < 0) + goto clean;

Casting away const multiple times here. THis can be avoided by changing const char *argv[7]; to be const char * const argv[7]; I find the out-of-order initialization of argv indicies here a little confusing. Even though it'd be duplicating a little, I think it'd be clearer to have if (create) { const char *const argv[] = { VIR_AA_HELPER_PATH, "-c", "-u", profile, NULL }; ret = virExec(conn, argv, NULL, NULL, &child, pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS); } else if (disk && disk->src) { const char *const argv[] = { VIR_AA_HELPER_PATH, "-r", "-u", profile, "-f", disk->src, NULL }; ret = virExec(conn, argv, NULL, NULL, &child, pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS); } else { const char *const argv[] = { VIR_AA_HELPER_PATH, "-r", "-u", profile, NULL }; ret = virExec(conn, argv, NULL, NULL, &child, pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS); } if (ret < 0) goto cleanup;

+ + + /* parent continues here */ + if (safewrite(pipefd[1], xml, strlen(xml)) < 0) { + virReportSystemError(conn, errno, "%s", _("unable to write to pipe")); + goto clean; + } + close(pipefd[1]); + rc = 0; + + rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + virSecurityReportError(conn, VIR_ERR_ERROR, + _("Unexpected exit status from virt-aa-helper " + "%d pid %lu"), + WEXITSTATUS(status), (unsigned long)child); + rc = -1; + } + + clean: + VIR_FREE(xml); + + failed: + if (pipefd[0] > 0) + close(pipefd[0]); + if (pipefd[1] > 0) + close(pipefd[1]); + + return rc; +}

+ +static int +remove_profile(const char *profile) +{ + const char *argv[5]; + int rc = -1; + + argv[0] = VIRT_AA_HELPER_PATH; + argv[1] = (char *) "-R"; + argv[2] = (char *) "-u"; + argv[3] = profile; + argv[4] = NULL;

To preserve const-ness & remove hardcoded array indexes, this could be written as const char * const argv[] = { VIRT_AA_HELPER_PATH, "-R", "-u", profile, NULL };

+ + if (virRun(NULL, argv, NULL) == 0) + rc = 0; + + return rc; +} + +/* + * profile_name is buffer to hold name and len is how many bytes in the + * buffer + */ +static int +get_profile_name(virConnectPtr conn, virDomainObjPtr vm, + char *profile_name, const size_t len) +{ + virDomainPtr dom = NULL; + int rc = -1; + + if (len < PROFILE_NAME_SIZE) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", _("profile_name has wrong size")); + return rc; + } + profile_name[0] = '\0'; + strcat(profile_name, AA_PREFIX); + + /* generate the profile name */ + dom = virGetDomain(conn, vm->def->name, vm->def->uuid); + dom->id = vm->def->id; + + if (virDomainGetUUIDString(dom, + &profile_name[strlen(profile_name)]) != 0) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", _("could not find uuid for VM")); + return rc; + } + + return 0; +}

I'm not really following why this code needs to call out to the public API just to get the UUID into string format. It not clear that this is sufficiently checking the allocate char * is large enough for the UUID string result. I think I'd prefer to see this returning a string allocated on the heap, rather than the caller's stack. The whole method could thus be simplified down to just char *uuidstr[VIR_UUID_STRING_BUFLEN]; char *ret; virUUIDFormat(vm->def->uuid, uuidstr); if (virAsprintf(&ret, "%s%s", AA_PREFIX, uuidstr) < 0) return NULL; return ret;

+/* returns -1 on error or profile for libvirtd is unconfined, 0 if complain + * mode and 1 if enforcing + */ +static int +use_apparmor(void) +{ + char libvirt_daemon[PATH_MAX]; + int rc = -1; + ssize_t len = 0; + + if ((len = readlink("/proc/self/exe", libvirt_daemon, + PATH_MAX - 1)) < 0) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + "%s", _("could not find libvirtd")); + return rc; + } + libvirt_daemon[len] = '\0'; + + if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) + return rc; + + return profile_status(libvirt_daemon, 1); +}

Is this the best way to check if app armour is enabled on a host ? It ought to be possible to use the app armour security driver for protecting guests regardless of whether the libvirtd daemon itself has a profile surely.

+static int +AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) +{ + int rc = -1; + char profile_name[PROFILE_NAME_SIZE]; + + profile_name[0] = '\0'; + + if ((vm->def->seclabel.label) || + (vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", + _("security label already defined for VM")); + return rc; + } + + if (get_profile_name(conn, vm, profile_name, sizeof(profile_name)) < 0) + return rc; + + /* if the profile is not already loaded, then load one */ + if (profile_loaded(profile_name) < 0) { + if (load_profile(conn, profile_name, vm, NULL) < 0) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("cannot generate AppArmor profile " + "\'%s\'"), profile_name); + return rc; + } + } + + vm->def->seclabel.label = strndup(profile_name, strlen(profile_name)); + if (!vm->def->seclabel.label) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", _("cannot generate AppArmor profile " + "name (label)")); + goto err; + } + + /* set imagelabel the same as label (but we won't use it) */ + vm->def->seclabel.imagelabel = strndup(profile_name, + strlen(profile_name)); + if (!vm->def->seclabel.imagelabel) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", + _("cannot generate AppArmor profile name " + "(imagelabel)")); + goto err; + }

These two strndup failures should also be virReportOOMError()

+ + vm->def->seclabel.model = strdup(SECURITY_APPARMOR_NAME); + if (!vm->def->seclabel.model) { + virReportOOMError(conn); + goto err; + } + + rc = 0; + goto done; + + err: + remove_profile(profile_name); + VIR_FREE(vm->def->seclabel.label); + VIR_FREE(vm->def->seclabel.imagelabel); + VIR_FREE(vm->def->seclabel.model); + done: + return rc; +} +

diff -Naurp libvirt.orig/tools/Makefile.am libvirt/tools/Makefile.am --- libvirt.orig/tools/Makefile.am 2009-09-22 12:51:57.000000000 -0500 +++ libvirt/tools/Makefile.am 2009-09-25 17:00:13.000000000 -0500 @@ -51,6 +51,33 @@ virsh_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(LIBXML_CFLAGS) \ $(READLINE_CFLAGS) + +if WITH_SECDRIVER_APPARMOR +bin_PROGRAMS += virt-aa-helper

Since this program is only really intended for libvirt to call rather than general admin usage I think it'd be beter to list it as libexec_PROGRAMS, so it ends up in /usr/libexec instead of /usr/bin I think I'd actually have it in the src/security/ directory, since we usually keep libexec programs alongside the driver code that's using them, just have tools/ for end user programs

+/* + * run an apparmor_parser command + */ +static int +parserCommand(const char *profile_name, const char cmd) +{ + const char *argv[4]; + char flag[3]; + char profile[PATH_MAX]; + + if (strchr("arR", cmd) == NULL) { + vah_error(NULL, 0, "invalid flag"); + return -1; + } + + snprintf(flag, 3, "-%c", cmd); + + if (snprintf(profile, PATH_MAX, "%s/%s", + APPARMOR_DIR "/libvirt", profile_name) > PATH_MAX - 1) { + vah_error(NULL, 0, "profile name exceeds maximum length"); + return -1; + } + + if (!virFileExists(profile)) { + vah_error(NULL, 0, "profile does not exist"); + return -1; + } + + argv[0] = (char *) "/sbin/apparmor_parser"; + argv[1] = flag; + argv[2] = profile; + argv[3] = NULL;

Same issue with const-ness as earlier examples

+ + if (virRun(NULL, argv, NULL) != 0) { + vah_error(NULL, 0, "failed to run apparmor_parser"); + return -1; + } + + return 0; +}

+static int +valid_uuid(const char *uuid) +{ + int i; + + if (strlen(uuid) != PROFILE_NAME_SIZE - 1) + return -1; + + if (STRNEQLEN(AA_PREFIX, uuid, strlen(AA_PREFIX))) + return -1;

This one can just be if (!STRPREFIX(uuid, AA_PREFIX)) return -1;

+ + for (i = strlen(AA_PREFIX); i < PROFILE_NAME_SIZE - 1; i++) { + if (uuid[i] == '-') + continue; + if (!c_isxdigit(uuid[i])) + return -1; + } + return 0; +}

How about just using virUUIDParse to check UUID validity, such as char rawuuid[VIR_UUID_BUFLEN]; if (virUUIDParse(uuid + strlen(AA_PREFIX), rawuuid) < 0) return -1; return 0;

+ +static int +valid_name(const char *name) +{ + /* just try to filter out any dangerous characters in the name that can be + * used to subvert the profile */ + char *bad = (char *) " /[]*";

Please declare it const, rather than casting away const.

+ int i; + + if (strlen(name) == 0 || strlen(name) > PATH_MAX - 1) + return -1; + + for (i = 0; i < strlen(bad); i++) + if (strchr(name, bad[i]) != NULL) + return -1;

FYI, there'a convenient libc function for checking a string for bad characters - strspn, you can use it in this way: if (strspn(name, bad) != strlen(name)) return -1;

+static int +valid_path(const char *path, const bool readonly) +{ + struct stat sb; + int i; + int npaths = 20; + char **restricted; + int rc = -1; + + if (path == NULL || strlen(path) > PATH_MAX - 1) { + vah_error(NULL, 0, "bad pathname"); + return rc; + } + + /* Don't allow double quotes, since we use them to quote the filename + * and this will confuse the apparmor parser. + */ + if (strchr(path, '"') != NULL) + return 1; + + if (VIR_ALLOC_N(restricted, npaths) < 0) { + vah_error(NULL, 0, "could not allocate memory for paths"); + return rc; + } + + restricted[0] = (char *) "/bin/"; + restricted[1] = (char *) "/etc/"; + restricted[2] = (char *) "/lib"; + restricted[3] = (char *) "/lost+found/"; + restricted[4] = (char *) "/proc/"; + restricted[5] = (char *) "/sbin/"; + restricted[6] = (char *) "/selinux/"; + restricted[7] = (char *) "/sys/"; + restricted[8] = (char *) "/usr/bin/"; + restricted[9] = (char *) "/usr/lib"; + restricted[10] = (char *) "/usr/sbin/"; + restricted[11] = (char *) "/usr/share/"; + restricted[12] = (char *) "/usr/local/bin/"; + restricted[13] = (char *) "/usr/local/etc/"; + restricted[14] = (char *) "/usr/local/lib"; + restricted[15] = (char *) "/usr/local/sbin/"; + /* these paths are ok for readonly */ + restricted[16] = (char *) "/boot/"; + restricted[17] = (char *) "/vmlinuz"; + restricted[18] = (char *) "/initrd"; + restricted[19] = (char *) "/initrd.img"; + + /* don't check paths ok for readonly */ + if (readonly) + npaths = 16;

More const issues. Rather than manually declaring the array size, and indexing manually its safer to initialize the array at time of declaration with all the strings. I'd probably go for separate arrays for the read-write vs read-only difference

+ + if (!virFileExists(path)) + vah_warning("path does not exist, skipping file type checks"); + else { + if (stat(path, &sb) == -1) + goto end; + + rc = 1; + switch (sb.st_mode & S_IFMT) { + case S_IFDIR: + goto end; + break; + case S_IFIFO: + goto end; + break; + case S_IFSOCK: + goto end; + break; + default: + break; + } + } + + for (i = 0; i < npaths; i++) { + if (strlen(path) < strlen(restricted[i])) + continue; + + if (STREQLEN(path, restricted[i], strlen(restricted[i]))) { + rc = 1; + goto end; + } + } + rc = 0; + + end: + VIR_FREE(restricted); + return rc; +}

+ + +static int +vah_add_file(virBufferPtr buf, const char *path, const char *perms) +{ + char *tmp = NULL; + int rc = -1; + bool readonly = true; + + if (path == NULL) + return rc; + + if (virFileExists(path)) { + if ((tmp = realpath(path, NULL)) == NULL) { + vah_error(NULL, 0, path); + vah_error(NULL, 0, " could not find realpath for disk"); + return rc; + } + } else + if ((tmp = strdup(path)) == NULL) + return rc; + + if (strchr(perms, 'w') != NULL) + readonly = false; + + rc = valid_path(tmp, readonly); + if (rc != 0) { + if (rc > 0) { + vah_error(NULL, 0, path); + vah_error(NULL, 0, " skipped restricted file"); + } + goto clean; + } + + virBufferVSprintf(buf, " \"%s\" %s,\n", tmp, perms); + + if (virBufferError(buf)) { + vah_error(NULL, 0, "failed to allocate file buffer"); + rc = -1; + }

It is not really neeccesssar to check virBufferError here. The idea is that you can call virBufferVSprintf() as many times as you like in a row without any checking, and then only check virBufferError at the end when you've finishing writing to the buffer.

+ + clean: + free(tmp); + + return rc; +} + +static int +get_files(vahControl * ctl) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBuffer uuid_buf = VIR_BUFFER_INITIALIZER; + int i; + int rc = -1; + char *uuid; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + /* verify uuid is same as what we were given on the command line */ + virUUIDFormat(ctl->def->uuid, uuidstr); + virBufferVSprintf(&uuid_buf, "%s%s", AA_PREFIX, uuidstr); + if (virBufferError(&uuid_buf)) { + vah_error(ctl, 0, "failed to allocate file buffer"); + return rc; + } + uuid = virBufferContentAndReset(&uuid_buf);

Using virBuffer for a single printf here is a little overkill - the virAsprintf() function would be more than sufficient

+ if (STRNEQ(uuid, ctl->uuid)) { + vah_error(ctl, 0, "given uuid does not match XML uuid"); + goto clean; + } + + for (i = 0; i < ctl->def->ndisks; i++) + if (ctl->def->disks[i] && ctl->def->disks[i]->src) + if (vah_add_file(&buf, ctl->def->disks[i]->src, "rw") != 0) + goto clean; + + for (i = 0; i < ctl->def->nserials; i++) + if (ctl->def->serials[i] && ctl->def->serials[i]->data.file.path) + if (vah_add_file(&buf, + ctl->def->serials[i]->data.file.path, "w") != 0) + goto clean; + + if (ctl->def->console && ctl->def->console->data.file.path) + if (vah_add_file(&buf, ctl->def->console->data.file.path, "w") != 0) + goto clean; + + if (ctl->def->os.kernel && ctl->def->os.kernel) + if (vah_add_file(&buf, ctl->def->os.kernel, "r") != 0) + goto clean; + + if (ctl->def->os.initrd && ctl->def->os.initrd) + if (vah_add_file(&buf, ctl->def->os.initrd, "r") != 0) + goto clean; + + if (ctl->def->os.loader && ctl->def->os.loader) + if (vah_add_file(&buf, ctl->def->os.loader, "r") != 0) + goto clean; + + /* needs patch to hostusb.h to work + for (i = 0; i < ctl->def->nhostdevs; i++) + if (ctl->def->hostdevs[i]) { + virDomainHostdevDefPtr hostdev = ctl->def->hostdevs[i]; + if (hostdev->source.subsys.type == + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { + usbDevice *usb = usbGetDevice(NULL, + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); + if (usb == NULL) + continue; + rc = vah_add_file(&buf, usb->path, "rw"); + usbFreeDevice(NULL, usb);

This is the bit of code that should be changed to use the usbDeviceFileIterate() method, since you can't assume there is only 1 path which is why we didn't make 'path' public.

+ if (rc != 0) + goto clean; + } + } + */ + + if (ctl->newdisk) + if (vah_add_file(&buf, ctl->newdisk, "rw") != 0) + goto clean;

If you remove the virBufferError check from the vah_add_file calls, it could just go in here

+ + rc = 0; + ctl->files = virBufferContentAndReset(&buf); + clean: + VIR_FREE(uuid); + return rc; +} +

Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.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, Oct 05, 2009 at 04:00:02PM -0500, Jamie Strandboge wrote:

Attached is an updated patch_2_apparmor_driver_updated.patch and patch_3_docs_updated.patch. Thanks for the review! These two patches along with the previous patch_1_reenable-nonfile-labels.patch pass syntax-check, make check (for the tests I added) and introduce no regressions over the previous patch. See below for inline comments. In addition to implenting your suggestions, the patch also now supports <readonly/> for disks and I defensively quote the pid, monitor and logfile. I also added ReserveSecurityLabel (a noop) along with stubs for SetSecurityHostdevLabel() and RestoreSecurityHostdevLabel(). [...] +static int +profile_status(const char *str, const int check_enforcing) +{ + char *content = NULL; + char *tmp = NULL; + char *etmp = NULL; + int rc = -1; + + /* create string that is '<str> \0' for accurate matching */ + if (virAsprintf(&tmp, "%s ", str) == -1) + return rc; + + if (check_enforcing != 0) { + /* create string that is '<str> (enforce)\0' for accurate matching */ + if (virAsprintf(&etmp, "%s (enforce)", str) == -1) { + VIR_FREE(tmp); + return rc; + } + } + + if (virFileReadAll(APPARMOR_PROFILES_PATH, MAX_FILE_LEN, &content) < 0) { + virReportSystemError(NULL, errno, + _("Failed to read AppArmor profiles list " + "\'%s\'"), APPARMOR_PROFILES_PATH);

----

+ if (check_enforcing != 0) + VIR_FREE(etmp); + goto clean;

+ } + + if (strstr(content, tmp) != NULL) + rc = 0; + if (check_enforcing != 0) { + if (rc == 0 && strstr(content, etmp) != NULL) + rc = 1; /* return '1' if loaded and enforcing */

+ VIR_FREE(etmp);

I would remove those 2 blocks and just VIR_FREE(etmp) in clean: since etmp is initialized to NULL, it's simpler.

+ } + + VIR_FREE(content); + clean: + VIR_FREE(tmp); + + return rc; +} + [...] +static int +profile_status_file(const char *str) +{ + char profile[PATH_MAX]; + char *content = NULL; + char *tmp = NULL; + int rc = -1; + int len; + + if (snprintf(profile, PATH_MAX, "%s/%s", APPARMOR_DIR "/libvirt", str) + > PATH_MAX - 1) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + "%s", _("profile name exceeds maximum length")); + }

rather than allocating a full page on the stack for profile, virAsprintf is probably simpler and more efficient but would need an unified exit to free it up.

+ if (!virFileExists(profile)) { + return rc; + } + + if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { + virReportSystemError(NULL, errno, + _("Failed to read \'%s\'"), profile); + return rc; + } + + /* create string that is ' <str> flags=(complain)\0' */ + if (virAsprintf(&tmp, " %s flags=(complain)", str) == -1) { + virReportOOMError(NULL); + goto clean; + } + + if (strstr(content, tmp) != NULL) + rc = 0; + else + rc = 1; + + VIR_FREE(tmp); + clean: + VIR_FREE(content); + + return rc; +} [...] +static int +load_profile(virConnectPtr conn, const char *profile, virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + int rc = -1, status, ret; + bool create = true; + char *xml = NULL; + int pipefd[2]; + pid_t child; + + if (pipe(pipefd) < -1) { + virReportSystemError(conn, errno, "%s", _("unable to create pipe")); + return rc; + } + + xml = virDomainDefFormat(conn, vm->def, VIR_DOMAIN_XML_SECURE); + if (!xml) + goto failed; + [...] + + clean: + VIR_FREE(xml); + + failed:

no need for a special failed: target, VIR_FREE(NULL) is fine. [...]

+static int +use_apparmor(void) +{ + char libvirt_daemon[PATH_MAX]; + int rc = -1; + ssize_t len = 0; + + if ((len = readlink("/proc/self/exe", libvirt_daemon, + PATH_MAX - 1)) < 0) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + "%s", _("could not find libvirtd")); + return rc; + } + libvirt_daemon[len] = '\0';

Hum, readlink() can be a bit nasty, I would suggest to use virFileResolveLink() here.

+ if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) + return rc; + + return profile_status(libvirt_daemon, 1); +} + +/* Called on libvirtd startup to see if AppArmor is available */ +static int +AppArmorSecurityDriverProbe(void) +{ + char template[PATH_MAX]; + + if (use_apparmor() < 0) + return SECURITY_DRIVER_DISABLE; + + /* see if template file exists */ + if (snprintf(template, PATH_MAX, "%s/TEMPLATE", + APPARMOR_DIR "/libvirt") > PATH_MAX - 1) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + "%s", _("template too large")); + return SECURITY_DRIVER_DISABLE; + }

again I would suggest to not allocate MAX_PATH on the stack, but using virAsprintf, but this would mean changing the function exit code.

+ if (!virFileExists(template)) { + virSecurityReportError(NULL, VIR_ERR_ERROR, + _("template \'%s\' does not exist"), template); + return SECURITY_DRIVER_DISABLE; + } + + return SECURITY_DRIVER_ENABLE; +} [...]

ACK for me on the changes to the library. I spotted a few possible improvements but this can be done in a later patch, no need to review everything again IMHO.

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 1969-12-31 18:00:00.000000000 -0600 +++ libvirt/src/security/virt-aa-helper.c 2009-10-05 15:22:59.000000000 -0500

For virt-aa-helper, some of the changes could be made there too like not allocate 4k on the stack and use virAsprintf or unify some of the exit labels, but since it's standalone it's not a big deal. ACK for tests

diff -Naurp libvirt.orig/docs/drvqemu.html.in libvirt/docs/drvqemu.html.in --- libvirt.orig/docs/drvqemu.html.in 2009-10-05 10:32:51.000000000 -0500 +++ libvirt/docs/drvqemu.html.in 2009-10-05 10:32:07.000000000 -0500 @@ -296,6 +296,73 @@

and ACK for doc and examples. 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/