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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list