On 2/19/20 10:40 AM, Peter Krempa wrote:
1b: base.raw probes as qcow2 (because of whatever the guest wrote there), using it as qcow2 is wrong - the guest will see corrupted data. What's more, if the probe sees it as qcow2 with backing file, and we open the backing file, it also has security implications.
We don't open the backing file in the proposed logic. That is specifically forbidden.
I agree that you contained the security risk by not following any file names mentioned in based.raw when interpreted as qcow2, but that does not mean you contained the data corruption risk.
Also in pre-blockdev configurations the same situation would happen as we allowed qemu to probe the backing file despite assuming the format to be raw. This was as we weren't able to tell qemu what the backing format is. This is fixed with -blockdev, so this scenario is exactly the same as it was before.
We assumed it to be raw, the question is whether qemu also assumed it to be raw, or qemu assumed it to be qcow2. If qemu assumed it to be qcow2, sVirt may have saved us from the assumption going haywire and opening forbidden files, but it did NOT save the guest from seeing corrupted data. If qemu assumed it to be raw, then all that was missing pre-blockdev was a way for us to tell qemu its assumptions were correct.
Scenario 2:
base.qcow2 <- wrap.qcow2
where wrap.qcow2 specifies backing of base.qcow2 but not the format. If we probe, we will always have just one outcome:
2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if base.qcow2 has a further backing image, the backing chain is now dependent on a probe.
Since 1b and 2a have the same probe result, but massively different data corruption and/or security concerns, it is NOT sufficient to claim that a probe was safe merely because "the probed image does not have any backing or data file". It is ONLY safe if the probe turns up raw. Any other probed format is inherently unsafe.
I disagree here. If the probe of 'base.qcow2' showed a backing file, we refuse startup right away. If it didn't show any backing file, we continue:
1) with old (pre-blockdev qemus) libvirt starts qemu with wrap.qcow2 as image. Qemu tries to open the backing file and probes it. Now if we've mis-detected that there is a backing file, we will depend on sVirt to save us. This scenario is how all of this was working until 2 months ago. It's because we've asumed that the format of 'base.qcow2' was raw, but started qemu. Since we didn't tell qemu what the format of 'base.qcow2' as it was impossible, we've relied on sVirt anyways.
This is the same as it was before.
So you're arguing that because qemu probes and treats the file as qcow2, even though we had assumed raw, but that the data actually was qcow2 (so the guest did not see data corruption), that we want to continue this practice by explicitly telling qemu that it is qcow2. Then this all boils down to "what does qemu do when it probes an image that does not result in raw". If qemu trusted the probe results anyway, then data corruption was already possible, but once the data corruption happens, the guest can no longer reverse the corruption nor cause further security damage (once qemu treats a previously-raw image as qcow2, the guest can no longer rewrite the qcow2 metadata). If qemu failed to use the image because it treated the image as raw, then libvirt's decision to tell qemu that the image is qcow2 will CAUSE data corruption. I recall that older qemu did blindly trust the probe results, but that there was discussion on the qemu list about patching things to warn about probes that resulted in anything other than raw. But I could not quickly find mailing list discussions or specific patches that mention what actually happens; the closest I got was: commit e4c8f2925d22584b2008aadea5c70e1e05c2a522 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Tue Nov 20 17:56:46 2018 +0000 iotests: fix nbd test 233 to work correctly with raw images The first qemu-io command must honour the $IMGFMT that is set rather than hardcoding qcow2. The qemu-nbd commands should also set $IMGFMT to avoid the insecure format probe warning.
2) with new qemu, we do the same, but start qemu and specifically tell it that "the backing format is qcow2 and that the image has no backing file. That way qemu doesn't even attempt to open anythting. This means that this scenario fixes any deployment without selinux, while keeping old semantics around.
If you tell qemu that something is qcow2, but qemu has ever in the past treated that file as raw, then you are forcing data corruption on the guest, even if you avoid a security issue of chasing further backing files from treating that image as qcow2.
We perform the image format detection and in the case that we were able to probe the format and the format does not specify a backing store (or doesn't support backing store) we can use this format.
Wrong. The condition needs to be:
If we probe the format, and the probe returns raw, then it is safe to use raw as the format.
That doesn't solve anythign then. The idea of this series is to relax the restrictions we've imposed after introducing blockdev to return the main semantics back to what we've allowed in pre-blockdev configurations.
The only way I see that might be safe to relax restrictions is if the filename heuristics give us a clue as to the user's intent. Data corruption is bad enough that we should never be the cause of it. A user with a broken setup deserves a decent error message that their setup is broken, and how to fix it (even so much as "we detected qcow2, but weren't sure if you might have had a raw file instead, so do this to tell us which one you meant"), but blindly picking one always creates a corner case where our choice is wrong.
Namely a user creates an overlay on top of single raw/qcow2 image and expects it to work. And it's not just random users, libguestfs and openstack also neglected to set the backing format.
Yes, and they are getting patched. Belatedly, but better late than never.
The price for this is that libvirt will need to keep the image format detector still current and working or replace it by invocation of qemu-img.
Maybe. Any format that qemu recognizes but libvirt does not risks a case where libvirt probes the image as raw but lets qemu re-probe the image and
That doesn't happen with blockdev. We dont' let qemu probe.
We are just shifting the burden on who causes the data corruption when a probe goes wrong - it used to be qemu, now it is libvirt.
I disagree with the logic here. What we really need is:
If the backing format was not specified, we probe to see what is there. If the result of that probe is raw, it is safe to treat the image as raw. If the result is anything else, we must report an error stating that what we probed could not be trusted unless the user adds an explicit backing format (either confirming that our probe was correct, or with the correct format overriding what we mis-probed).
As noted above, using this logic would be pointless. We are better off just reporting the error always if we also don't allow qcow2 without backing file to be used as it was previously used.
Then report the error always. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org