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

[Michal Privoznik]

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?

Michal