Hi, Jim,
On Thu, Feb 09, 2017 at 09:30:16AM -0700, Jim Fehlig wrote:
> Guido Günther wrote:
> > 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@xxxxxxxx>
> >> ---
> >> 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.
>
> Opps, thanks for catching that. I've removed it in my local branch.
>
> Do we need someone else with knowledge of the apparmor driver to review and ACK
> these changes?
It would be great if Jamie could comment as well in case we're
overlooking any details. Otherwise I think we're good to go.
Cheers,
-- Guido
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
