On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote: > On 1/7/25 08:23, Georgia Garcia wrote: > > 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. > > Have you considered, or experimented with, adding a "remove file" option to the > "replace" mode of virt-aa-helper? Figuring out the short name of the option > might be the most difficult part :-P. I didn't experiment with it because I thought it was a change too drastic to change the behavior of "Restore" to instead of regenerate the policy based on the xml, to read the policy, string match the drive (for example) being removed, remove that entry and rewrite the policy file. By maintaining current behavior for the most part I would also lower the risk of regressions. It might be possible but I'd have to look into more detail into all the "Restore" hooks to say for certain. > > > 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. > > I'm not super excited about this approach, but I'm also no apparmor expert. > Perhaps my preference for a '--remove-file' option to supplement '--add-file' is > not really possible or realistic with the current apparmor integration. > > Andrea also has some experience with apparmor and its libvirt support. He may > have better advice on fixing this issue. > Since there aren't hooks for removing permissions for files that were created by FD (domainSetSecurityImageFDLabel / domainSetSecurityTapFDLabel) I figured that separating them in a different file was the best approach but I'm open to changing it if it's more appropriate. Any feedback is welcome! Thank you, Georgia > Regards, > Jim > > > > > 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@xxxxxxxxxxxxx> > > --- > > 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 91c51f6395..907b01577c 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, > > @@ -245,10 +248,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); > > @@ -258,7 +262,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); > > @@ -268,6 +272,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) > > { > > @@ -388,7 +404,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); > > @@ -420,7 +436,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; > > } > > @@ -1074,9 +1090,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; > > @@ -1112,7 +1128,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 1626d5a89c..3a217fa3d1 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")); > > @@ -1356,10 +1358,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': > > @@ -1396,6 +1399,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; > > @@ -1445,9 +1451,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]); > > @@ -1479,13 +1492,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; > > @@ -1513,6 +1529,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) { > > @@ -1535,11 +1552,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) { > > @@ -1550,11 +1576,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) { > > @@ -1566,6 +1593,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); > > } > > } > > > > @@ -1578,6 +1606,7 @@ main(int argc, char **argv) > > /* cleanup */ > > if (rc != 0) { > > unlink(include_file); > > + unlink(include_runtime_file); > > if (ctl->cmd == 'c') > > unlink(profile); > > } >
