Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> On Fri, Aug 15, 2014 at 01:21:19PM +0200, Markus Armbruster wrote:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
>> >> On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
>> >> > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
>> >> > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
>> >> > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
>> >> > > > > Fixed size VPC images do not have a footer, hence the current 
>> >> > > > > probe
>> >> > > > > function will fail and QEMU will fall back to the raw_bsd
>> >> > > > > driver, which
>> >> > > > > is
>> >> > > > > not the correct behaviour. The specification of the format says 
>> >> > > > > that
>> >> > > > > fixed
>> >> > > > > size images have a footer as the last 512 bytes of the
>> >> > > > > file. The footer
>> >> > > > > is
>> >> > > > > exactly the same as the header would be in the case of dynamically
>> >> > > > > growing
>> >> > > > > images.
>> >> > > > > 
>> >> > > > > For this, we need to read the last 512 bytes of the
>> >> > > > > image, however the
>> >> > > > > current mechanics predominantly read the first 2048 bytes
>> >> > > > > and pass that
>> >> > > > > as a buffer to the probe functions. Solve this by passing the
>> >> > > > > BlockDriverState to the probe functions, hence giving
>> >> > > > > them a chance to
>> >> > > > > read
>> >> > > > > the extra bytes they might need.
>> >> > > > 
>> >> > > > I hesitate to add patches that extend image format probing.  For the
>> >> > > > past few years we have always recommended that image files
>> >> > > > should not be
>> >> > > > probed.
>> >> > > > 
>> >> > > > Image probing is prone to security issues because a
>> >> > > > malicious guest can
>> >> > > > modify a raw or vpc image by putting another image format header at
>> >> > > > sector 0.  The next time QEMU opens the image it will
>> >> > > > detect a different
>> >> > > > format.  One evil trick is to refer to a file on the host
>> >> > > > file system as
>> >> > > > the backing file, now you can read any file that the QEMU process 
>> >> > > > has
>> >> > > > access to.
>> >> > 
>> >> > Yea, good point. The current state of probing is actually quite bad,
>> >> > just take a look at dmg_probe in block/dmg.c :-(
>> >> > 
>> >> > > > 
>> >> > > > Probing also complicates live migration.  The source host
>> >> > > > still has the
>> >> > > > image file open and may write to it.  The destination host shouldn't
>> >> > > > even read from the image file before handover to avoid file cache
>> >> > > > coherency issues.
>> >> > > > 
>> >> > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
>> >> > > > (especially by adding more I/Os).
>> >> > 
>> >> > Even though, my series would have only added one extra I/O in the case
>> >> > of failing VPC images, I have to admit you are right.
>> >> > 
>> >> > > 
>> >> > > For 2.2, maybe we should limit probing to only certain operations 
>> >> > > (e.g.
>> >> > > qemu-img info) - or perhaps just remove the capability altogether, or
>> >> > > at least start phasing it out with a warning message that automatic
>> >> > > format detection is deprecated and may be unsafe.
>> >> > > 
>> >> > 
>> >> > Considering the fact that most open functions already check the magic
>> >> > numbers, and they do a lot better/safer job at it, we could just swap
>> >> > the probe functions with the open ones and just insert an fprintf
>> >> > when format is not specified doing what Jeff suggested.
>> >> > 
>> >> > Any objections to this?
>> >> > 
>> >> > (This would also solve the VPC-fixed-size bug, since vpc_open already
>> >> > checks the footer if the header is not found)
>> >> >
>> >> 
>> >> I was proposing actually going a bit further than this, and not
>> >> allowing automatic format detection at all, with an exception for
>> >> 'qemu-img info'. In the interim, until that is in place and it is
>> >> removed, warn with a deprecation message.
>> >
>> > No, we can't do this. It would immediately destroy -hda and similar
>> > convenience options and make the command line really hard to use even
>> > for simple cases. I usually call qemu manually and I specify format
>> > basically _never_, because it would like double the length of my command
>> > line (okay, not quite, but my command lines are usually very short) and
>> > I know what I'm doing and I'm running trusted guests.
>> >
>> > Plus, there are probably many scripts out there that rely on it.
>> >
>> > A more reasonable approach would be to just forbid probing raw and
>> > raw-like formats like VHD fixed (the rest should be safe), but I think
>> > the impact of this would still be too bad.
>> 
>> I think we're doing our users a disservice by sticking to the fatally
>> flawed probing.  "Broken by default" is just wrong, and "convenience" is
>> no excuse.
>> 
>> I believe we can retain 90% of the convenience without the flaws, by
>> defaulting format based on file meta-data only: name and struct stat.
>
> I worry that will subtly alter current behavior in bad ways.  For

Oh, yes, it does alter current behavior.

> instance, take this image chain:
>
>     qemu-img create -f qcow2 foo.img 1G
>     qemu-img create -f qcow2 -b foo.img bar.img 1G
>
>     qemu-kvm -drive file=bar.img,format=qcow2
>
>
> If I understand correctly what you are proposing, that means that
> qemu-kvm would detect 'foo.img' as raw, while current behavior is to
> detect it as 'qcow2'.

Correct.  Educate people to call it foo.qcow2 already, or to add
format=raw.

> Although if we do that in conjunction with what Kevin proposed (forbid
> probing on raw), it would behave 'properly', and bail out before doing
> something bad.  That could be OK.

The obvious way forward is to start warning folks when we pick a format
based on image contents, unless it matches the pick based on image name
and stat.  Do that for a year or three, then switch over.

If we'd done that back when we discovered the flaw (CVE-2008-2004), our
users would've been safe by default for several years now.

[...]