[PATCH] Don't require secdrivers to implement .domainMoveImageMetadata

Christian Ehrhardt christian.ehrhardt at canonical.com
Tue May 26 07:46:26 UTC 2020 

On Mon, May 25, 2020 at 3:25 PM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> On 5/25/20 1:14 PM, Christian Ehrhardt wrote:
> > On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn at redhat.com> wrote:
> >>
> >> On 5/18/20 10:02 AM, Erik Skultety wrote:
> >>
> >>> Yes, I know, what I meant by "unrelated" here was just the fact that in order
> >>> to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
> >>> next time :). It's true that with the first hunk the second becomes redundant,
> >>> but I still feel like the NOP driver should cover the full spectrum of
> >>> operations we support, but maybe I'm trying to be overly cautious here.
> >>>
> >>
> >> Well, it doesn't implement everything already. But okay, I have ACK for
> >> the important hunk so I will push only that one.
> >
> > Hi Michal,
> > while debugging a Ubuntu bug report I found that this fix will
> > mitigate the warning you wanted to fix.
> > But overall there still is an issue with the labeling introduced by
> > [1][2] in >=5.6.
> >
> > The situation (related to this fix, hence replying in this context) is
> > the following.
> > Ubuntu (as an example) builds and has build with --without-attr.
> >
> > That will make the code have !HAVE_LIBATTR which was fine in the past.
> > But with your code added the LP bug [3] identified an issue.
> >
> > What happens is that in stacked security it will iterate on:
> > - virSecurityStackMoveImageMetadata (for the stacking itself)
> > - 0x0 (apparmor)
> > - virSecurityDACMoveImageMetadata
> >
> > The fix discussed here fixes the warning emitted by the apparmor case like:
> > "this function is not supported by the connection driver"
> >
> > But when iterating further on a build that has no attr support we
> > encounter the following (e.g. at guest shutdown):
> >
> > libvirtd[6320]: internal error: child reported (status=125):
> > libvirtd[6320]: Unable to remove disk metadata on vm testguest from
> > /var/lib/uvtool/libvirt/images/testguest.qcow (disk target vda)
> >
> > I found that this is due to:
> > qemuBlockRemoveImageMetadata (the one that emits the warning)
> >    -> qemuSecurityMoveImageMetadata
> >      -> virSecurityManagerMoveImageMetadata
> >        -> virSecurityDACMoveImageMetadata
> >          -> virSecurityDACMoveImageMetadataHelper
> >            -> virProcessRunInFork (spawns subprocess)
> >              -> virSecurityMoveRememberedLabel
> >
> > Since this is built without HAVE_LIBATTR the following will happen
> >
> > 461 if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) {
> > (gdb) n
> > 462 if (errno == ENOSYS || errno == ENOTSUP) {
> > (gdb) p errno
> > $32 = 38
> >
> > And that is due to !HAVE_LIBATTR which maps the implementation onto:
> >
> > 4412 #else /* !HAVE_LIBATTR */
> > 4413
> > 4414 int
> > 4415 virFileGetXAttrQuiet(const char *path G_GNUC_UNUSED,
> > 4416 const char *name G_GNUC_UNUSED,
> > 4417 char **value G_GNUC_UNUSED)
> > 4418 {
> > 4419 errno = ENOSYS;
> > 4420 return -1;
> > 4421 }
> >
> > Due to that we see the two messages reported above
> > a) internal errors -> for the subprocess that failed
> > b) "Unable to remove disk metadata" -> but this time due to DAC
> > instead of apparmor in the security stack
>
> Thank you for your deep analysis.
>
> >
> > I'm not sure what you'd prefer Michal, maybe an early RC=0 exit in
> > virSecurityMoveRememberedLabel in case of !HAVE_LIBATTR?
> > Con: That would still fork the process to do nothing then
> > Pro: It would but be a small change in just one place
> >
> > Since you did all the related changes I thought I report the case and
> > leave it up to you Michal, what do you think?
>
> Yes, a naive fix would be something like this:
>
> diff --git i/src/security/security_dac.c w/src/security/security_dac.c
> index bdc2d7edf3..7b95a6f86d 100644
> --- i/src/security/security_dac.c
> +++ w/src/security/security_dac.c
> @@ -1117,6 +1117,12 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid
> G_GNUC_UNUSED,
>
>       ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src,
> data->dst);
>       virSecurityManagerMetadataUnlock(data->mgr, &state);
> +
> +    if (ret == -2) {
> +        /* Libvirt built without XATTRS */
> +        ret = 0;
> +    }
> +
>       return ret;
>   }
>
>
> But as you correctly say, it will still lead to an unnecessary spawn of
> a process that will do NOP (basically). I think a better fix would be to
> not require .domainMoveImageMetadata callbacks (i.e. the one from NOP
> driver could be then removed) and set the DAC/SELinux callbacks if and
> only if HAVE_LIBATTR; alternatively, make those functions be NOP if
> !HAVE_LIBATTR.
>
> On the other hand, every time we relabel a path outside of /dev, we
> technically spawn unnecessary because only /dev lives inside a private
> NS. Everything else is from the parent NS. For instance, there is no
> need to spawn only to chown("/var/lib/libvirt/images/fedora.qcow2").
>
> Therefore I might have a slight preference for the naive fix, but I will
> leave it up to you. Or do you want me to sent the patch?

Hi,
as mentioned on IRC yesterday I was giving this a look.
While looking deeper at the naive fix I was derailed and probably have
given it too much second though for the naive implementation.

TL;DR:
- we should be ok with the naive fix as suggested

Details:
The low level implementations all have an ifdef on HAVE_LIBATTR to
return -1 and set ENOSYS.
- virFileSetXAttr
- virFileGetXAttrQuiet (the only one not pushing to virReportSystemError)
- virFileRemoveXAttr

Checking their callers I saw a mixed situation.

"rc not checked" => means with out libattr support these will behave
as if it failed.
"rc checked and delivering -2" => means it compares to ENOSYS/ENOTSUP
and returns -2

Converting ENOTSUP to rc=-1:
- virSecurityAddTimestamp
  -> virFileSetXAttr (rc not checked)
- virSecurityRemoveTimestamp
  -> virFileRemoveXAttr (rc not checked)

Early check via virFileGetXAttrQuiet returning -2:
- virSecurityValidateTimestamp
  -> virFileGetXAttrQuiet ("rc checked and delivering -2")
- virSecuritySetRememberedLabel
  -> virFileGetXAttrQuiet ("rc checked and delivering -2")
  -> 2 x virFileSetXAttr (rc not checked)
- virSecurityGetRememberedLabel
  -> virFileGetXAttrQuiet ("rc checked and delivering -2")
  -> virFileSetXAttr (rc not checked)
  -> virFileRemoveXAttr (rc not checked)
- virSecurityMoveRememberedLabel
  -> 3 x virFileGetXAttrQuiet ("rc checked and delivering -2")
  -> 3 x virFileSetXAttr (rc not checked)
  -> 3 x virFileRemoveXAttr (rc not checked)
  -> 3 x virFileRemoveXAttr (rc ignored without consequence)

It might have been meant to use virFileGetXAttrQuiet (no error report)
as probing function to early exit.
It is usually used early and would thereby return -2.

But can we be sure that e.g. virSecurityAddTimestamp and
virSecurityRemoveTimestamp are never called without HAVE_LIBATTR.
Since those would just ignore the ENOTSUP/ENOSYS and make it a bad
rc=1. They might trigger a similar issue to the one I reported. Do
they need the same "if ENOTSUP/ENOSYS then RC=-2"?
I checked the callers and as of today it seems to be only
virSecurityGetRememberedLabel and virSecuritySetRememberedLabel wich
would before probe via virFileGetXAttrQuiet.
So might be safe on these.

But furthermore even when -2 is passed back, as my case has shown the
RC of virSecurityMoveRememberedLabel wasn't checked for RC==-2 to be
handled gracefully in virSecurityDACMoveImageMetadataHelper.
I checked all the callers of the above list that returns -2, they all
ignore the special case.
So instead of just virSecurityDACMoveImageMetadataHelper as discussed
we might need the same/similar change in:

- virSecuritySELinuxRememberLabel
- virSecuritySELinuxRecallLabel
- virSecuritySELinuxMoveImageMetadataHelper
(SELinux without XATTR isn't really a thing I guess - so we might skip
these, but I'm not sure)

- virSecurityDACRememberLabel -> rc=-2 covered in virSecurityDACSetOwnership
- virSecurityDACRecallLabel -> rc=-2 covered in
virSecurityDACRestoreFileLabelInternal

I was first afraid if "just fixing"
virSecurityDACMoveImageMetadataHelper might fix the case I've found
while there might be more cases just waiting to trigger with different
use cases. But it seems we are good in that regard.
But after a long trip through the code it seems just
virSecurityDACMoveImageMetadataHelper should be ok.
It comes down to officially submitting the change Michal suggested ...

@Michal - if anything above rings a bell and "should work differently"
speak up here or on the patch that follows and feel free to provide a
better fix then adapting to whatever is different than I assumed.


> Michal
>


--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

More information about the libvir-list mailing list