Re: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>

On Fri, Feb 03, 2017 at 10:32:12AM -0700, Jim Fehlig wrote: > If the apparmor security driver is loaded/enabled and domain config > contains a <seclabel> element whose type attribute is not 'apparmor', > starting the domain fails when attempting to label resources such > as tap FDs. > > Many of the apparmor driver entry points attempt to retrieve the > apparmor security label from the domain def, returning failure if > not found. Functions such as AppArmorSetFDLabel fail even though > domain config contains an explicit 'none' secuirty driver, e.g. > > <seclabel type='none' model='none'/> > > Change the entry points to succeed if the domain config <seclabel> > is not apparmor. This matches the behavior of the selinux driver. > > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; > --- > src/security/security_apparmor.c | 58 ++++++++++++---------------------------- > 1 file changed, 17 insertions(+), 41 deletions(-) > > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > index ad50b08..f871e60 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -289,10 +289,7 @@ reload_profile(virSecurityManagerPtr mgr, > virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef( > def, SECURITY_APPARMOR_NAME); > > - if (!secdef) > - return rc; > - > - if (!secdef->relabel) > + if (!secdef || !secdef->relabel) > return 0; > > if ((profile_name = get_profile_name(def)) == NULL) > @@ -435,7 +432,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > SECURITY_APPARMOR_NAME); > > if (!secdef) > - return -1; > + return 0; > > if ((secdef->type == VIR_DOMAIN_SECLABEL_STATIC) || > (secdef->type == VIR_DOMAIN_SECLABEL_NONE)) > @@ -495,10 +492,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr, > { > virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, > SECURITY_APPARMOR_NAME); > - if (!secdef) > - return -1; > - > - if (!secdef->relabel) > + if (!secdef || !secdef->relabel) > return 0; > > /* Reload the profile if stdin_path is specified. Note that > @@ -559,12 +553,11 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > { > virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, > SECURITY_APPARMOR_NAME); > - if (!secdef) > - return -1; > - > - VIR_FREE(secdef->model); > - VIR_FREE(secdef->label); > - VIR_FREE(secdef->imagelabel); > + if (secdef) { > + VIR_FREE(secdef->model); > + VIR_FREE(secdef->label); > + VIR_FREE(secdef->imagelabel);\ The trailing slash can be dropped here. The rest of th code looks good to me.