Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 29.10.2014 um 14:54 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> > Instead, let me try once more to sell my old proposal [1] from the
>> > thread you mentioned:
>> >
>> >> 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?
>> >
>> > Attacks the problem where it arises instead of trying to detect the
>> > outcome of it, and works in whatever way it is nested in the BDS graph
>> > and whatever way is used to address the image file.
>> >
>> > Kevin
>> >
>> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02548.html
>> 
>> Arbitrarily breaking the virtual hardware that way is incompatible, too.
>> It's a bigger no-no for me than changing user interface corner cases or
>> deciding what to do with a file based on its file name extension.
>
> It is arbitrarily breaking theoretical cases, while your patch is less
> arbitrarily breaking common cases. I strongly prefer the former.

I challenge "theoretical" as well as "common".

For "theoretical", see below.

As to "common": are unrecognizable image names really common?  I doubt
it.  If I'm wrong, I expect users to complain about my warning, and then
we can reconsider.

> Nobody will ever want to write a qcow2 header into their boot sector for
> just running a guest:
>
> 0:   51                      push   %cx
> 1:   46                      inc    %si
> 2:   49                      dec    %cx
> 3:   fb                      sti
>
> This code doesn't make any sense. It's similar for other formats. And if
> they really have some odd reason to do that, the fix is easy: Either
> don't use raw, or pass an explicit format=raw.

A disk needn't start with a PC-style boot sector.  Even on a PC.  Not
every disk needs to be bootable, or even partitioned.  Databases exist
that like to work on raw devices.  Giving them /dev/sdb instead of a
whole-disk partition /dev/sdb1 may not be the smartest thing to do, but
it *works* with real hardware, so why not with virtual hardware?

You either have to prevent *any* writing of the first 2048 bytes (the
part that can be examined by a bdrv_probe() method, or your have to
prevent writing anything a probe recognizes, or the user has to specify
the format explicitly.

If you do the former, you're way outside the realm of "theoretical".

If you do the latter, the degree of "theoreticalness" depends on the
union of the patterns you prevent.  Issues:

1. Anthony's method of checking a few known signatures is fragile.  The
only obviously robust way to test "is probing going to find something
other than 'raw'?" is running the probes.  Feasible.

2. Whether the union of patterns qualifies as "theoretical" for all our
targets is not obvious, and whether it'll remain "theoretical" for all
future formats and target machines even less.

3. A write access that works just fine in one QEMU version can be
rejected by another version that recognizes more formats.  Or probes the
same formats differently.

4. Rejecting a write fails *late*, and looks like hardware failure to
the guest.  With imperfect guest software, this risks data corruption.

(4) is a deal breaker for me.

(3) adds a cherry on top.  I care about command line compatibility, but
I care about guest ABI stability a whole lot more.

I'm utterly unwilling to risk breaking a running guest's hardware just
so that users can continue to call their qcow2 images
"I want my pony.dammit".

>> Anthony tried something similar (commit 79368c8), but couldn't get it
>> right (commit 8b33d9e).
>
> The discussion back then: http://patchwork.ozlabs.org/patch/58980/
>
> The problem with Anthony's code was that he didn't handle a qiov
> correctly that had unaligned members. With today's block layer, this is
> not a big deal to implement correctly. We're running coroutines instead
> of AIO callbacks and we don't have to do all the manual qiov fixing
> magic that Anthony had in his patch, util/iov.c provides all you need.
>
> I'll send out an RFC series that implements this.

I'm strongly opposed to this idea.