proc and fd_path are allocated but never freed. Fix by using g_autofree instead. Fixes: b9757fea30785a92aa95ea675b9bc371e4fb2e8c Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com&gt; --- src/security/security_apparmor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 07e95ec81d..7092724563 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1107,8 +1107,8 @@ AppArmorSetFDLabel(virSecurityManager *mgr, virDomainDef *def, int fd) { - char *proc = NULL; - char *fd_path = NULL; + g_autofree char *proc = NULL; + g_autofree char *fd_path = NULL; virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); -- 2.34.1

Moving towards full adoption of GLib APIs in the AppArmor code. Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com&gt; --- src/security/security_apparmor.c | 41 ++++--------- src/security/virt-aa-helper.c | 100 ++++++++++--------------------- 2 files changed, 45 insertions(+), 96 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7092724563..9e578b2526 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -115,37 +115,28 @@ profile_loaded(const char *str) static int profile_status_file(const char *str) { - char *profile = NULL; - char *content = NULL; - char *tmp = NULL; - int rc = -1; + g_autofree char *profile = NULL; + g_autofree char *content = NULL; + g_autofree char *tmp = NULL; int len; profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", str); if (!virFileExists(profile)) - goto failed; + return -1; if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { virReportSystemError(errno, _("Failed to read \'%1$s\'"), profile); - goto failed; + return -1; } /* create string that is ' <str> flags=(complain)\0' */ tmp = g_strdup_printf(" %s flags=(complain)", str); if (strstr(content, tmp) != NULL) - rc = 0; - else - rc = 1; - - failed: - VIR_FREE(tmp); - VIR_FREE(profile); - VIR_FREE(content); - - return rc; + return 0; + return 1; } /* @@ -218,7 +209,7 @@ static int use_apparmor(void) { int rc = -1; - char *libvirt_daemon = NULL; + g_autofree char *libvirt_daemon = NULL; if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -232,7 +223,7 @@ use_apparmor(void) return 1; if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) - goto cleanup; + return rc; /* First check profile status using full binary path. If that fails * check using profile name. @@ -245,8 +236,6 @@ use_apparmor(void) rc = -1; } - cleanup: - VIR_FREE(libvirt_daemon); return rc; } @@ -948,7 +937,7 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd G_GNUC_UNUSED) { - char *in = NULL, *out = NULL; + g_autofree char *in = NULL, *out = NULL; int ret = -1; virSecurityLabelDef *secdef; @@ -969,11 +958,11 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, out = g_strdup_printf("%s.out", dev_source->data.file.path); if (virFileExists(in)) { if (reload_profile(mgr, def, in, true) < 0) - goto done; + return ret; } if (virFileExists(out)) { if (reload_profile(mgr, def, out, true) < 0) - goto done; + return ret; } ret = reload_profile(mgr, def, dev_source->data.file.path, true); break; @@ -993,9 +982,6 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, break; } - done: - VIR_FREE(in); - VIR_FREE(out); return ret; } @@ -1081,12 +1067,11 @@ AppArmorSetPathLabel(virSecurityManager *mgr, bool allowSubtree) { int rc = -1; - char *full_path = NULL; + g_autofree char *full_path = NULL; if (allowSubtree) { full_path = g_strdup_printf("%s/{,**}", path); rc = reload_profile(mgr, def, full_path, true); - VIR_FREE(full_path); } else { rc = reload_profile(mgr, def, path, true); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..601f2d2581 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -146,9 +146,8 @@ vah_info(const char *str) static int parserCommand(const char *profile_name, const char cmd) { - int result = -1; char flag[3]; - char *profile; + g_autofree char *profile = NULL; int status; int ret; @@ -163,7 +162,7 @@ parserCommand(const char *profile_name, const char cmd) if (!virFileExists(profile)) { vah_error(NULL, 0, _("profile does not exist")); - goto cleanup; + return -1; } else { const char * const argv[] = { "/sbin/apparmor_parser", flag, profile, NULL @@ -175,23 +174,18 @@ parserCommand(const char *profile_name, const char cmd) (WIFEXITED(status) && WEXITSTATUS(status) != 0)) { if (ret != 0) { vah_error(NULL, 0, _("failed to run apparmor_parser")); - goto cleanup; + return -1; } else if (cmd == 'R' && WIFEXITED(status) && WEXITSTATUS(status) == 234) { vah_warning(_("unable to unload already unloaded profile")); } else { vah_error(NULL, 0, _("apparmor_parser exited with error")); - goto cleanup; + return -1; } } } - result = 0; - - cleanup: - VIR_FREE(profile); - - return result; + return 0; } /* @@ -201,18 +195,17 @@ static int update_include_file(const char *include_file, const char *included_files, bool append) { - int rc = -1; int plen, flen = 0; int fd; - char *pcontent = NULL; - char *existing = NULL; + g_autofree char *pcontent = NULL; + g_autofree char *existing = NULL; const char *warning = "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n"; if (virFileExists(include_file)) { flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing); if (flen < 0) - return rc; + return -1; } if (append && virFileExists(include_file)) @@ -223,38 +216,31 @@ update_include_file(const char *include_file, const char *included_files, plen = strlen(pcontent); if (plen > MAX_FILE_LEN) { vah_error(NULL, 0, _("invalid length for new profile")); - goto cleanup; + return -1; } /* only update the disk profile if it is different */ if (flen > 0 && flen == plen && STREQLEN(existing, pcontent, plen)) { - rc = 0; - goto cleanup; + return 0; } /* write the file */ if ((fd = open(include_file, O_CREAT | O_TRUNC | O_WRONLY, 0644)) == -1) { vah_error(NULL, 0, _("failed to create include file")); - goto cleanup; + return -1; } if (safewrite(fd, pcontent, plen) < 0) { /* don't write the '\0' */ VIR_FORCE_CLOSE(fd); vah_error(NULL, 0, _("failed to write to profile")); - goto cleanup; + return -1; } if (VIR_CLOSE(fd) != 0) { vah_error(NULL, 0, _("failed to close or write to profile")); - goto cleanup; + return -1; } - rc = 0; - - cleanup: - VIR_FREE(pcontent); - VIR_FREE(existing); - - return rc; + return 0; } /* @@ -572,7 +558,7 @@ caps_mockup(vahControl * ctl, const char *xmlStr) { g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - char *arch; + g_autofree char *arch = NULL; if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_definition)"), "domain", &ctxt, NULL, false))) { @@ -598,7 +584,6 @@ caps_mockup(vahControl * ctl, const char *xmlStr) ctl->arch = virArchFromHost(); } else { ctl->arch = virArchFromString(arch); - VIR_FREE(arch); } return 0; @@ -683,15 +668,15 @@ get_definition(vahControl * ctl, const char *xmlStr) static int vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive) { - char *tmp = NULL; int rc = -1; bool readonly = true; bool explicit_deny_rule = true; char *sub = NULL; - char *perms_new = NULL; - char *pathdir = NULL; - char *pathtmp = NULL; - char *pathreal = NULL; + g_autofree char *tmp = NULL; + g_autofree char *perms_new = NULL; + g_autofree char *pathdir = NULL; + g_autofree char *pathtmp = NULL; + g_autofree char *pathreal = NULL; if (path == NULL) return rc; @@ -728,7 +713,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive if ((pathreal = realpath(pathdir, NULL)) == NULL) { vah_error(NULL, 0, pathdir); vah_error(NULL, 0, _("could not find realpath")); - goto cleanup; + return rc; } tmp = g_strdup_printf("%s%s", pathreal, pathtmp); } @@ -752,7 +737,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive vah_error(NULL, 0, path); vah_error(NULL, 0, _("skipped restricted file")); } - goto cleanup; + return rc; } if (tmp[strlen(tmp) - 1] == '/') @@ -769,13 +754,6 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive virBufferAsprintf(buf, " \"%s/\" r,\n", tmp); } - cleanup: - VIR_FREE(pathdir); - VIR_FREE(pathtmp); - VIR_FREE(pathreal); - VIR_FREE(perms_new); - VIR_FREE(tmp); - return rc; } @@ -791,36 +769,28 @@ vah_add_file_chardev(virBuffer *buf, const char *perms, const int type) { - char *pipe_in; - char *pipe_out; - int rc = -1; + g_autofree char *pipe_in = NULL; + g_autofree char *pipe_out = NULL; if (type == VIR_DOMAIN_CHR_TYPE_PIPE) { /* add the pipe input */ pipe_in = g_strdup_printf("%s.in", path); if (vah_add_file(buf, pipe_in, perms) != 0) - goto clean_pipe_in; + return -1; /* add the pipe output */ pipe_out = g_strdup_printf("%s.out", path); if (vah_add_file(buf, pipe_out, perms) != 0) - goto clean_pipe_out; - - rc = 0; - clean_pipe_out: - VIR_FREE(pipe_out); - clean_pipe_in: - VIR_FREE(pipe_in); + return -1; } else { /* add the file */ if (vah_add_file(buf, path, perms) != 0) return -1; - rc = 0; } - return rc; + return 0; } static int @@ -1467,8 +1437,8 @@ main(int argc, char **argv) vahControl _ctl = { 0 }; vahControl *ctl = &_ctl; int rc = -1; - char *profile = NULL; - char *include_file = NULL; + g_autofree char *profile = NULL; + g_autofree char *include_file = NULL; off_t size; bool purged = 0; @@ -1511,7 +1481,7 @@ main(int argc, char **argv) if (ctl->cmd == 'D') unlink(include_file); } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { - char *included_files = NULL; + g_autofree char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; if (ctl->cmd == 'c' && virFileExists(profile)) @@ -1573,7 +1543,7 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ if (ctl->cmd == 'c' || purged) { - char *tmp = NULL; + g_autofree char *tmp = NULL; #if defined(WITH_APPARMOR_3) const char *ifexists = "if exists "; #else @@ -1591,7 +1561,6 @@ main(int argc, char **argv) vah_error(ctl, 0, _("could not create profile")); unlink(include_file); } - VIR_FREE(tmp); } if (rc == 0 && !ctl->dryrun) { @@ -1607,14 +1576,9 @@ main(int argc, char **argv) unlink(profile); } } - cleanup: - VIR_FREE(included_files); } - + cleanup: vahDeinit(ctl); - VIR_FREE(profile); - VIR_FREE(include_file); - exit(rc == 0 ? EXIT_SUCCESS : EXIT_FAILURE); } -- 2.34.1

On 1/7/25 01:56, Jim Fehlig via Devel wrote: It is, because it leads to smaller diffs in the future. Please do make the change before pushing these. Michal

On 11/13/24 07:28, Georgia Garcia wrote: The changes here are also needed in usr.sbin.virtqemud.in. Regards, Jim

Some rules are generated dynamically during boot and added to the AppArmor policy. An example of that is macvtap devices that call the AppArmorSetFDLabel hook to add a rule for the tap device path. Since this information is dynamic, it is not available in the xml config, therefore whenever a "Restore" hook is called, the entire profile is regenerated by virt-aa-helper based only the information from the VM definition, so the dynamic/runtime information is lost. This patch stores the dynamically generated rules in a new file called libvirt-uuid.runtime_files which is included by the AppArmor policy. This file should exist while the domain is running and should be reloaded automatically whenever there's a restore operation. These rules only make sense when the VM is running, so the file is removed when the VM is shutdown. Note that there are no hooks for restoring FD labels, so that information is not removed from the set of rules while the domain is running. Closes: https://gitlab.com/libvirt/libvirt/-/issues/692 Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com&gt; --- src/security/security_apparmor.c | 38 +++++++++++++++++++-------- src/security/virt-aa-helper.c | 45 ++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 9e578b2526..28f263ddd4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -147,7 +147,8 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, const char *profile, virDomainDef *def, const char *fn, - bool append) + bool append, + bool runtime) { bool create = true; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -173,6 +174,8 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, } else { virCommandAddArgList(cmd, "-f", fn, NULL); } + if (runtime) + virCommandAddArgList(cmd, "-t", NULL); } virCommandAddEnvFormat(cmd, @@ -243,10 +246,11 @@ use_apparmor(void) * NULL. */ static int -reload_profile(virSecurityManager *mgr, - virDomainDef *def, - const char *fn, - bool append) +reload_runtime_profile(virSecurityManager *mgr, + virDomainDef *def, + const char *fn, + bool append, + bool runtime) { virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef( def, SECURITY_APPARMOR_NAME); @@ -256,7 +260,7 @@ reload_profile(virSecurityManager *mgr, /* Update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(mgr, secdef->imagelabel, def, fn, append) < 0) { + if (load_profile(mgr, secdef->imagelabel, def, fn, append, runtime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot update AppArmor profile \'%1$s\'"), secdef->imagelabel); @@ -266,6 +270,18 @@ reload_profile(virSecurityManager *mgr, return 0; } +/* reload the profile, adding read/write file specified by fn if it is not + * NULL. + */ +static int +reload_profile(virSecurityManager *mgr, + virDomainDef *def, + const char *fn, + bool append) +{ + return reload_runtime_profile(mgr, def, fn, append, false); +} + static int AppArmorSetSecurityHostdevLabelHelper(const char *file, void *opaque) { @@ -386,7 +402,7 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, secdef->model = g_strdup(SECURITY_APPARMOR_NAME); /* Now that we have a label, load the profile into the kernel. */ - if (load_profile(mgr, secdef->label, def, NULL, false) < 0) { + if (load_profile(mgr, secdef->label, def, NULL, false, false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot load AppArmor profile \'%1$s\'"), secdef->label); @@ -418,7 +434,7 @@ AppArmorSetSecurityAllLabel(virSecurityManager *mgr, /* Reload the profile if incomingPath is specified. Note that GenSecurityLabel() will have already been run. */ if (incomingPath) - return reload_profile(mgr, def, incomingPath, true); + return reload_runtime_profile(mgr, def, incomingPath, true, true); return 0; } @@ -1071,9 +1087,9 @@ AppArmorSetPathLabel(virSecurityManager *mgr, if (allowSubtree) { full_path = g_strdup_printf("%s/{,**}", path); - rc = reload_profile(mgr, def, full_path, true); + rc = reload_runtime_profile(mgr, def, full_path, true, true); } else { - rc = reload_profile(mgr, def, path, true); + rc = reload_runtime_profile(mgr, def, path, true, true); } return rc; @@ -1109,7 +1125,7 @@ AppArmorSetFDLabel(virSecurityManager *mgr, return 0; } - return reload_profile(mgr, def, fd_path, true); + return reload_runtime_profile(mgr, def, fd_path, true, true); } static char * diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 601f2d2581..98cf9411ea 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -71,6 +71,7 @@ typedef struct { virArch arch; /* machine architecture */ char *newfile; /* newly added file */ bool append; /* append to .files instead of rewrite */ + bool runtime; /* file should be added to .runtime_files */ } vahControl; static int @@ -110,6 +111,7 @@ vah_usage(void) " Extra File:\n" " -f | --add-file <file> add file to a profile generated from XML\n" " -F | --append-file <file> append file to an existing profile\n" + " -t | --runtime file is valid only during runtime\n" "\n"), progname); puts(_("This command is intended to be used by libvirtd and not used directly.\n")); @@ -1350,10 +1352,11 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) { "replace", 0, 0, 'r' }, { "remove", 0, 0, 'R' }, { "uuid", 1, 0, 'u' }, + { "runtime", 0, 0, 't' }, { 0, 0, 0, 0 }, }; - while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:F:", opt, + while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:F:t", opt, &idx)) != -1) { switch (arg) { case 'a': @@ -1390,6 +1393,9 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) PROFILE_NAME_SIZE) < 0) vah_error(ctl, 1, _("error copying UUID")); break; + case 't': + ctl->runtime = true; + break; default: vah_error(ctl, 1, _("unsupported option")); break; @@ -1439,9 +1445,16 @@ main(int argc, char **argv) int rc = -1; g_autofree char *profile = NULL; g_autofree char *include_file = NULL; + g_autofree char *include_runtime_file = NULL; off_t size; bool purged = 0; +#if defined(WITH_APPARMOR_3) + const char *ifexists = "if exists "; +#else + const char *ifexists = ""; +#endif + if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { fprintf(stderr, _("%1$s: initialization failed\n"), argv[0]); @@ -1473,13 +1486,16 @@ main(int argc, char **argv) profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", ctl->uuid); include_file = g_strdup_printf("%s/%s.files", APPARMOR_DIR "/libvirt", ctl->uuid); + include_runtime_file = g_strdup_printf("%s/%s.runtime_files", APPARMOR_DIR "/libvirt", ctl->uuid); if (ctl->cmd == 'a') { rc = parserLoad(ctl->uuid); } else if (ctl->cmd == 'R' || ctl->cmd == 'D') { rc = parserRemove(ctl->uuid); - if (ctl->cmd == 'D') + if (ctl->cmd == 'D') { unlink(include_file); + unlink(include_runtime_file); + } } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { g_autofree char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -1507,6 +1523,7 @@ main(int argc, char **argv) if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; } else { + virBufferAsprintf(&buf, " #include %s<libvirt/%s.runtime_files>\n", ifexists, ctl->uuid); if (ctl->def->virtType == VIR_DOMAIN_VIRT_QEMU || ctl->def->virtType == VIR_DOMAIN_VIRT_KQEMU || ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) { @@ -1529,11 +1546,20 @@ main(int argc, char **argv) /* (re)create the include file using included_files */ if (ctl->dryrun) { - vah_info(include_file); + if (ctl->runtime) + vah_info(include_runtime_file); + else + vah_info(include_file); vah_info(included_files); rc = 0; } else if (ctl->def->virtType == VIR_DOMAIN_VIRT_LXC) { rc = 0; + } else if (ctl->runtime) { + /* runtime should only update include_runtime_file */ + if ((rc = update_include_file(include_runtime_file, + included_files, + ctl->append)) != 0) + goto cleanup; } else if ((rc = update_include_file(include_file, included_files, ctl->append)) != 0) { @@ -1544,11 +1570,12 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ if (ctl->cmd == 'c' || purged) { g_autofree char *tmp = NULL; -#if defined(WITH_APPARMOR_3) - const char *ifexists = "if exists "; -#else - const char *ifexists = ""; -#endif + + /* ideally libvirt-uuid.files and + * libvirt-uuid.runtime_files should be in libvirt-uuid.d/ + * and the directory should be included instead, but how + * to deal with running domains when the libvirt-uuid + * profile is not recreated? */ tmp = g_strdup_printf(" #include %s<libvirt/%s.files>\n", ifexists, ctl->uuid); if (ctl->dryrun) { @@ -1560,6 +1587,7 @@ main(int argc, char **argv) ctl->def->virtType)) != 0) { vah_error(ctl, 0, _("could not create profile")); unlink(include_file); + unlink(include_runtime_file); } } @@ -1572,6 +1600,7 @@ main(int argc, char **argv) /* cleanup */ if (rc != 0) { unlink(include_file); + unlink(include_runtime_file); if (ctl->cmd == 'c') unlink(profile); } -- 2.34.1