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

[Jeff Cody]

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
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'.

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.

> 
> This breaks down for things like QCOW2-formatted logical volumes.  But
> those things are exactly *not* the things casual users need.
> 
> It also breaks down when users call their QCOW2 images foo.vmdk, but I'd
> call that a feature :)