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

[Jeff Cody]

On Fri, Aug 15, 2014 at 12:55:19PM +0200, Kevin Wolf wrote:
> 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. 
>

You are right, we would need a way to specify the image format for the
convenience options (and remove a lot of the convenience).  Perhaps
that is a show-stopper right away.

The other big area of impact is image files that have metadata
specifying a backing file, but not the backing file format.  For
instance, this usage still probes when opening 'foo.qcow2':
    qemu-img create -f qcow2 foo.qcow2 1G
    qemu-img create -f qcow2 bar.qcow2 -b foo.qcow2

    qemu-kvm -drive file=bar.qcow2,format=qcow2


As an aside - I've heard us developers say multiple places that we
really recommend against image probing, but our command line options
and user documentation don't reflect that - at least not strongly.
(For qemu-img, the documentation even says image format is "guessed
automatically in most 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.
> 

Yes, I do the same, and it is pretty convenient.  This change would
be somewhat of a pita.

> 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 made some patches to add in a deprecation message, and an additional
option so 'qemu-img info' could bypass that message.

Things immediately noticed:

* qemu-io didn't have a way to specify image format easily, so that
  needed to be added.
* qemu-img didn't have a way to specify image format for some options;
  that needed added as well.
* pretty much every qemu io test needed to be modified, to explicitly
  specify image format.  I'm about 25% through those.

And of course, convenience options like -hda spit out the deprecation
warning - which I think is probably a good thing.  Here is what I made
it say:

      fprintf(stderr, "Format autodetection is deprecated and may be "
              "removed in future versions.  Image format autodetection "
              "is not reliable; some image formats (e.g. raw) may "
              "masquerade as other image formats.  This could lead to "
              "system data loss or leaks.\n");
  

If we think doing this is a good thing, I'll continue modifying the
qemu-iotests.  Otherwise, I'll drop it.

Allowing qemu-io to specify image format easily is probably a good
idea either way.