[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer |
Date: |
Fri, 16 Jul 2010 18:06:01 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Anthony Liguori <address@hidden> writes:
> 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.
Yes, users don't like having to specify the "obvious".
> 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.
Note: if we stop right here, the security hole is patched, but use of
raw images requires explicit specification of format.
> 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.
Doesn't this require an image that can grow? What about host block
devices?
> 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?
I'm not convinced it's a good idea. It's clearly a less bad idea,
though :)
It avoids guest-visible lossage, and that's good.
There's still host-visible lossage: as soon as we redirect sector 0, the
image isn't raw anymore, and accessing it with non-qemu tools (say
losetup + kpartx) no longer works. You need to know what QEMU did to
your no-longer-raw image to work around the lossage (say losetup -o
512).
>> 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.
I still think guessing the format from the file name is a better
way to spare users from having to specify formats.
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, (continued)
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Anthony Liguori, 2010/07/14
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Markus Armbruster, 2010/07/15
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Anthony Liguori, 2010/07/15
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Kevin Wolf, 2010/07/15
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Anthony Liguori, 2010/07/15
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Kevin Wolf, 2010/07/16
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Stefan Hajnoczi, 2010/07/16
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Stefan Hajnoczi, 2010/07/16
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Anthony Liguori, 2010/07/16
- Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Kevin Wolf, 2010/07/16
Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Anthony Liguori, 2010/07/14
Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Aurelien Jarno, 2010/07/14
Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer, Kevin Wolf, 2010/07/15