On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
> [adding qemu]
Adding Daniel as he objected to qemu-img.
>
> On 2/19/20 12:57 PM, Peter Krempa wrote:
[...]
> > Additionally I think that we could just get rid of the copy of the image
> > detection copy in libvirt and replace it by invocation of qemu-img. That
> > way we could avoid any discrepancies in the detection process in the
> > first place.
>
> Now there's an interesting thought. Since data corruption occurs when there
> is disagreement about which mode to use, getting libvirt out of the probing
> business by deferring all decisions to qemu-img info is a smart move - if
> qemu says an image probes as qcow2 (in an environment where probing is
> safe), then libvirt passing an explicit qcow2 to qemu for guest usage (in an
> environment where probing is not safe) will at least see the same
> guest-visible data. Less code to maintain in libvirt, and no chance for a
> mismatch between the two projects on which format a probe should result in.
I raised the use of qemu-img to Daniel and he disagreed with use of
qemu-img in libvirt for doing the probing on multiple reasons:
- qemu-img instantiates many data structures relevant to the format so
it has a huge attack surface
- performance of spawning extra processes would be way worse
While at least from the point of view of VM startup both can be
challenged this adds a complete new orthogonal dimension to the problem
I'm attempting to fix.
I'll reiterate the historical state of the problem because I think it's
important:
Pre-blockdev:
- we internally assumed that if the image format of an backing image
was not present in the overlay it is 'raw'
- this influenced security labelling but not actually how qemu viewed
or probed the file. If it was qcow2 probed as qcow2 qemu opened it
as qcow2 possibly even including the backing file if selinux or
other mechanism didn't prevent it.
post-blockdev:
- the assumption of 'raw' would now be expressed into the qemu
configuration. This assumption turned into data corruption since we
no longer allowed qemu to probe the format and forced it as raw.
- fix was to always require the format to be recorded in the overlay
- this made users unhappy who neglected to record the format into the
overlay when creating it manually
Now since qemu didn't discourage the creation of overlays without format
there still are many users which will inevitably hit this problem when
used with libvirt.
My proposal tries to mitigate the regressions in behaviour in the valid
and secure use cases. (If the image whose format we detect doesn't have
a backing image)
This comes at a trade-off though. As Eric pointed out, if the format
probed by libvirt's internal code disagrees with qemu's format we are
getting into the image corruption region.
As a mitigation to the above I suggested using qemu-img to probe but
that's a complex change and as mentioned above not really welcome
upstream.
Now this adds an interresting dimension to this problem. If libvirt
forces the users to specify the image format, and the users don't know
it they will probe. So we are basically making this a problem of
somebody else. [2] As you can see in that patch, it uses 'qemu-img'
anyways and also additionally actually allows the chain to continue
deeper! [3]
A partial relief to the image detection problem is that qemu would
refuse to start if an non-qcow2 image is used in qcow2, thus we really
only run into problems if qcow2 is mis-detected as raw.
This boils down to whether we want to accept some possibility of image
corruption in trade for avoiding regression of behaviour in the secure
cases as well as management apps and users not having to re-invent when
probing an image is actually safe.
Finally I think we should either decide to fix it in this release, or
stick with the error message forever. Fixing it later will not make
much sense as many users already fixed their scripts and we'd just add
back the trade-off of possible image corruption.
Peter
[1] If e.g. the security subsystem of the host didn't forbid the use of
the backing file such a qcow2 qemu would happily open it.
[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00013.html
[3] As implemented in [2] the backing image is not checked whether it
has a backing file or not but the format is probed, which way result
into accessing the backing chain of the probed image.
Prior to this detection, it would be prevented by sVirt or alternatively
also by libvit itself in -blockdev mode when this patch would be
accepted.
