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

[Jeff Cody]

On Fri, Aug 15, 2014 at 03:37:56PM +0200, Kevin Wolf wrote:
> Am 15.08.2014 um 14:14 hat Jeff Cody geschrieben:
> > 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.
> 
> I think it's very close to a show-stopper anyway.
> 
> But it wouldn't hurt to allow something like -cdrom test.iso,format=raw
> (or any other -drive options), that already saves some typing compared
> to -drive.
> 
> > 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
> 
> Ouch, you're right. If the above wasn't a show-stopper, this one is.
> Breaking all images with backing files so that you need to fix up whole
> backing file chains with rebase -u is not an option.
> 
> > 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.
> 
> No matter if we actually deprecate anything or not, these changes are
> definitely valuable.
> 
> > * 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.
> 
> I don't think this radical way works.
> 
> We can choose Markus's suggestion of using the file name to guess the
> format. I don't really like it much, but it seems like a fair compromise
> that doesn't hurt usability as much.
> 

Imagine the chain and cmdline described above, except someone created
images not with .qcow2 extension, but with .img extension.

If we go by file name, this happens when the user upgrades QEMU -

Old behavior:

  | foo.img | <--- | bar.img |
  | (qcow2) |      | (qcow2) |


New behavior:

  | foo.img | <--- | bar.img |
  | (raw)   |      | (qcow2) |

Which means the guest will almost certainly have data corruption.


> If we don't want this, we can approach the problem from a different
> angle: The problem is not probing per se, but that images probed as raw
> can be written to by guests in a way that the next time they are probed
> as something else.
> 
> What if we let the raw driver know that it was probed and then it
> enables a check that returns -EIO for any write on the first 2k if that
> write would make the image look like a different format?
>

I fear that would have even more ramifications, and lead to all sorts
of extra bugs and support (I can already see the block-commit bugs
now...).  And couldn't a guest still create a tricky
raw file, when format=raw was explicitly specified?  It would just
have to wait until some time that it was opened by probing.