Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer

[Anthony Liguori]

On 07/15/2010 10:19 AM, Markus Armbruster wrote:
> Anthony Liguori<address@hidden>  writes:
>
>    
> > On 07/14/2010 01:43 PM, Christoph Hellwig wrote:
> >      
> > > Err, strong NACK.  Please don't start messing with the contents of the
> > > data plane, we're getting into real trouble there.  It's perfectly
> > > valid for a guest to create an image inside an image, and with hardware
> > > support for nested virtualization I guess this use case will become
> > > rather common, just as it already is on S/390 with VM.
> > >
> > >        
> > Then we have to remove block format probing.
> >
> > The two things are fundamentally incompatible.
> >      
> I agree with Christoph: changing guest writes is a big no-no, and
> changing them silently is even worse.
>    

I do sympathize.  The problem is we're already doing this.  This patch 
simply changes the behavior to not be a security problem.  I've 
committed it to attempt to resolve that security problem.  However, we 
still have a problem and I don't consider the issue closed.

> I could perhaps accept EIO.  Elsewhere in this thread you wrote that you
> rejected that approach because "it would trigger the stop-on-error
> behavior and the result would be far too difficult for a management
> tool/person to deal with."  I think that would be *far* superior in
> fact: it fails spectacularly, immediately and safely instead of silently
> corrupting disk contents.
>    

There's really nothing wrong with this type of write, so EIO doesn't 
solve the problem.  While we can argue whether writing zeros or EIO is a 
"better bad" solution, let's try to figure out a good solution.

> The real problem in need of fixing is the unsafe default.  You wrote
> that "most users want block probing".  I disagree.  Users want to set up
> drives with as little hassle as possible.  If format is optional, and
> appears to work, why bother specifying it?

I really think specifying the format is a burden that is nice to avoid.

I have another idea that I hope will solve the problem in a more 
complete way.  The fundamental issue is that it's impossible to probe 
raw images reliably.  We can probe qcow2, vmdk, etc but not raw.

So, let's do the following: have raw_probe() always fail.  Probing 
shouldn't be a heuristic, it should be an absolute.  We can't prove it's 
a raw image, so we should always fail.

To accomodate current use-cases with raw, let's introduce a new format 
called "probed_raw".  probed_raw's semantics will be the following:

The signature of a probed_raw will be ~{'QFI\xfb', 'VMDK', 'COWD', 
'OOOM', ...}.  If the signature is 'QRAW', then instead of reading the 
first sector at offset 0, we read the first sector at offset LENGTH.  If 
the signature is 'QRAW', LENGTH is computed by calculating FILE_SIZE - 512.

For probed_raw, write requests to sector 0 are checked.  If the first 
four bytes is an invalid probed_raw signature or QRAW, we write a QRAW 
signature to file offset 0 and copy the first sector to the end of the 
file redirecting reads and writes to the end of file.

An approach like this has the following properties:

1) We can make the bdrv_probe check 100% reliable and return a boolean.
2) In the cases where we known format=raw, none of this code is ever 
invoked.
3) probed_raw images usually look exactly like raw images in most cases
4) In the degenerate cases, probe_raw images are still mountable in the 
normal way.
5) Even after the QRAW signature is applied, if the guest writes a valid 
signature, we can truncate the file and make it appear as a normal raw 
image.

Christoph/Markus/Stefan, does this seem like a more reasonable approach?

Regards,

Anthony Liguori

>    That they get an unsafe
> default that way is a big surprise to them.  And I can't blame them!
> Users can reasonably expect programs not to trap them.
>
> If we want to let users define drives without having to specify the
> format, we can guess the format from the file name.
>