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

[Stefan Hajnoczi]

On Thu, Jul 15, 2010 at 5:20 PM, Anthony Liguori <address@hidden> wrote:
> 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?

It took me a little while to figure out how your scheme works.  I like
that the check for sector 0 writes is moved out of the generic I/O
code path and into its own module.  The probed_raw format could be
easily dropped later if the decision is made to stop probing
altogether.

It would be simpler to avoid the QRAW signature and sector 0
redirection by simply failing dodgy writes to sector 0 with EIO.  You
said that would be confusing to users, but if we have no good way to
present errors to the user, then that is a different problem that
needs to be addressed anyway.

What we're talking about here is similar to "boot sector virus
protection" that BIOSes implement(ed?).

Stefan

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