[libvirt] [PATCH for v5.3.0 10/17] security: Remember owner only for top level image

[Cole Robinson]

On 3/28/19 11:04 AM, Michal Privoznik wrote:
> Here is the problem: If all disks had XATTRs (i.e. domains using
> them were started with owner remembering turned on) then
> refcounting implemented in XATTRs would work nicely and we could
> set the whole backing chain and restore it later. But world is
> not that simple. As soon as there is one domain that was started
> with the feature turned off (or simply by older libvirt), the
> XATTR refounting does not reflect the actual number of uses by
> running domains and therefore any attempt to restore might cut
> off the old domain.
> 
> There is no simple way around this. Except artificially turning
> the feature off for the rest of the backing chain.
> 

Okay so I studied this some more, here's my understanding.

VM has disk foo.qcow2 with a backing image backing.qcow2. We start the
VM, foo.qcow2 gets an svirt_image_t:... exclusive label. backing.qcow2
which is readonly for the VM and may be shared with other VMs, gets the
readonly label virt_content_t

With label remembering added, we need to consider the case of VMs that
were started without any remembering xattrs applied, so like from a
previous libvirt version, or maybe a VM started with remembering or
labeling disabled. In those cases, we should skip the xattr remembering
altogether. The problem is, we don't have any way to really detect this
scenario or its not important enough to implement.

In the case of the top foo.qcow2 image, even if it's in use by some
pre-remembering scenario, because our VM is requesting exclusive access
to it anyways we don't need to care about any back compat, so we set our
labels and use xattrs from here on out. That parts easy.

But the readonly backing.qcow2 could validly be in use by
pre-remembering VMs, and that's not feasible to detect one way or the
other. So for correctness we assume backing.qcow2 is pre-remembering and
maintain that behavior. In other words, we always skip manipulating
xattr refcounts for backing images. Which seems reasonable to me.

So for the approach and the code here:

Reviewed-by: Cole Robinson <crobinso at redhat.com>

But the follow on from that explanation is that this isn't really a
distinction between top vs backing images, it's exclusive vs shared
resources. Anything that's plausibly shareable we can't use xattrs
(unless we come up with a different approach).

Specifically it means that this logic should be applied to other shared
cases like disk->src->shared and disk->src->readonly. I tested a
<shareable/> disk and xattrs are incremented but never decremented, I
think because of conditions that skip label restore in
virSecuritySELinuxRestoreImageLabelInt.

That's a separate set of issues that should be fixed before the last
patch is applied.

Thanks,
Cole