[libvirt] backing file chains, -blockdev, and storage file autodetection

[Peter Krempa]

On Wed, Jan 08, 2020 at 13:34:14 +0000, Martin Wilck wrote:
> The recent change in libvirt to pass storage arguments to qemu via 
> "-blockdev", explicity passing backing file chain information rather
> than relying on qemu to figure it out, has bitten me quite painfully.

I'm sorry for causing this. I'm also sorry for going to sound too
dismissive later on.

> I've had the habit for years to use qcow2 with backing chains without
> specifying the backing file format explicitly, as information like the
> following was enough both for me and for qemu to figure out that the
> backing file was in qcow2 format:
> 
> # qemu-img info sles15-gm-cc.qcow2 
> image: sles15-gm-cc.qcow2
> file format: qcow2
> virtual size: 20 GiB (21474836480 bytes)
> disk size: 407 MiB
> cluster_size: 65536
> backing file: /mnt/vms/sles15-gm-base.qcow2
> 
> But if libvirt encounters a qemu-img file like this, it passes the
> backing file to qemu with "driver";"raw":
> 
> -blockdev '{"driver":"file","filename":"/mnt/vms/sles15-gm-base.qcow2","node-name":"libvirt-4-storage","auto-read-only":true,"discard":
> "unmap"}' \
> -blockdev '{"node-name":"libvirt-4-format","read-only":true,"driver":"raw","file":"libvirt-4-storage"}' \
> 
> The effect was that some of my VMs would refuse to start with a "no
> bootable disk" error message from the VM's BIOS. Others did boot (I
> believe this was because the boot sector had been modified in the top
> image in the backing file chain), and I'm quite grateful I did not
> attempt to boot them fully, because god knows what might have happened
> if the OS had later encountered garbage data from the backing file at
> some random point.
> 
> It took me half a day to figure out that this effect had been caused by
> the recent libvirt update.
> 
> I'm aware that documentation about this has been added recently 
> (https://libvirt.org/kbase/backing_chains.html (*)). Also I believe
> that current libvirt master (unlike 5.10.0) would refuse to start such
> images in the first place (
> https://bugzilla.redhat.com/show_bug.cgi?id=1588373), perhaps providing
> users with a better clue than before what was going wrong.
> 
> Meanwhile I've fixed my VM images by adding the backing file format
> tag, as suggested in the documentation. However I still think that this
> was quite a disruptive change and highly unexpected for users. IMO the
> default behavior shouldn't have been switched like this without
> appropriate warnings.

This is a good sounding idea but mostly pointless when attempting to
pull off.

If we'd add notes in the documentation, most people wouldn't read them
until stuff breaks. Similarly for putting warnings into the code. They
end up in the log file and not many people even bother to read them.

The only recourse would be: "we told you so a year ago in this document
you didn't read". The end result would be the same.

> The rationale given for not autodetecting the file format is "a
> malicious guest could rewrite the header of the disk leading to access
> of host files". I suppose a guest would need to manipulate a raw image
> to look like qcow2, qed or similar for this to happen (and set the
> backing file to "/etc/shadow", maybe?). Still the malicious guest would
> need to find a way to manipulate the data *on the backing store*,
> because the format of the topmost image is explicit anyway.
> Modifying the backing store could be difficult for the guest, because
> it's normally read-only and changes go only to the top layer. Or am I

You are right in cases when the backing image is always a backing image.

That can't be guaranteed though because the VM may use the image as the
only image in raw mode. Then when an overlay is created the image may
already be compromised. Obviously this requires the permission to do the
overlay, which can arguably not be granted. In general though this
scenario is possible.

In libvirt we already always record the image format when creating the
overlay for many years now.

> missing something? The opposite (manipulating qcow2 to look like raw)
> shouldn't be possible, IMO.

No this is impossible to do without compromising qemu or misconfiguring
the VM, so you are right.

> While I can't deny that an attack like this might be feasible, I am
> still wondering why this hasn't been an issue in past years (with qemu
> auto-detecting the backing file format).

Well, for distros using selinux this attack is mitigated by selinux
usage. That was also the reason why "assuming raw" was good enough. It
was also 'good enough' because there wasn't any other option.

It additionally created a whole lot of users which were already bitten
by this many years ago and now use the format correctly since we'd not
allow access otherwise when selinux is used.

> More importantly, perhaps the disruption caused by this change could be
> mitigated by allowing autodetection in certain cases (e.g. if the file
> name of the backing file indicates it's qcow2, as in the example
> above), or by providing a configuration option to enable it in
> environments (like mine, developer test environment) where evil guests
> are very unlikely.

The option to allow autodetection was removed some time ago with the
rationale that it's not worth even allowing reverting to an insecure
configuration.

While this will cause a disruption during upgrade, users will need to
modify the custom scripts. Unfortunately that would have to hapen in one
way or another anyways.

Ideally users should take advantage libvirt APIs to create overlays
since they always record the image format.

Also I'm not really sold on the idea of matching qcow2 in the image
name. It's just another bit of metadata, you can record it properly in
the overlay. (*)

Additionally there's a lot of wrong documentation on the internet which
doesn't call for specifying the backing format. Thus there's always a
group of things that will break.

Again, I'm sorry for sounding too dismissive, but in one way or another
I don't think this was avoidable. It could be partially mitigated if I
remembered to force the error when backing store format is missing in
5.10.

> Best regards,
> Martin
> 
> (*) This page is quite hard to find, googling for "libvirt backing
> chain" does not pick it up prominently just yet. Actually I only found

I added it just recently. Possibly the search engines didn't pick it up.

> this information by running "git grep" on the libvirt git repo.
> 
> -- 
> Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
> SUSE  Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer

* I will need to think about this for a bit first.