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

Michal Privoznik mprivozn at redhat.com
Tue May 26 10:38:18 UTC 2020


On 5/26/20 9:46 AM, Christian Ehrhardt wrote:
> 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.

Yeah, the idea is that at the beginning of every Rememember/Recall/Move 
Label call, there is virFileGetXAttrQuiet() which intentionally doesn't 
report error, not only because HAVE_LIBATTR might be unset, but also 
because the underlying FS might not support XATTRs. After that it is 
safe (and in fact desired) for every Set/Get XATTR call to return an error.

> 
> 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

Aren't these handled too, similarly to how DAC driver handles them?

> - virSecuritySELinuxMoveImageMetadataHelper

This one needs fixing, as I mention in my review of your patch.

> (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.

I think the patch is good.

Michal




More information about the libvir-list mailing list