Re: [Qemu-devel] [PATCH 0/3] virtio: Eliminate "exit(1)" upon invalid request in virtio-blk and virtio-scsi

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> On Wed, Apr 23, 2014 at 11:22:51AM +0800, Fam Zheng wrote:
>> On Tue, 04/22 22:12, Michael S. Tsirkin wrote:
>> > On Tue, Apr 22, 2014 at 04:55:14PM +0800, Fam Zheng wrote:
>> > > Today, buggy or malicous guests that submit invalid requests can
>> > > cause QEMU's
>> > > exit with an error message, which is not friendly to neither
>> > > user/admin nor
>> > > guest. When passing through a virtio device to a nested vm,
>> > > there is also an
>> > > D.O.S. vulnerability.
>> > > 
>> > > This series adds "broken" flag to VirtIODevice and allows device
>> > > emulation code
>> > > to set it if invalid data from guest is seen, and then decide
>> > > what to do with
>> > > the (invalid and/or further) requests, by checking the status of the 
>> > > flag.
>> > > 
>> > > Upon device reset, "broken" is cleared and the device comes back to 
>> > > normal
>> > > again.
>> > > 
>> > > In the patch 2 and 3, virtio-blk and virtio-scsi will just set
>> > > the broken flag,
>> > > and stop poping requests from virt queue. In other words, the
>> > > guest will find
>> > > the device inresponsive, the only way it can do is resetting the device.
>> > > 
>> > > Other virtio device types, as well as virtqueue core code, have
>> > > more exit(1)'s
>> > > to be converted, but could be done on top of this.
>> > > 
>> > > Thanks,
>> > > Fam
>> > > 
>> > 
>> > 
>> > It still seems trivially easy for a buggy guest to cause qemu to
>> > abort, e.g. by supplying an invalid physical address to write to.
>> > 
>> > OTOH it seems possible that killing the malicious guest early reduces our
>> > security attack surface.
>> > 
>> 
>> IMO, a buggy guest doing wrong operations is not necessarily the end of world
>> for QEMU, we shouldn't tear down the whole process because of invalid input.
>
> OK so we'd have to find a way to handle errors under memory APIs,
> in such a way that doesn't break all users.
> Maybe returning some dummy value on read and on ignoring writes?
> Another tricky issue would be device triggering infinite recursion by
> doing io on itself.

I'm not too familiar with the memory APIs, so take this with the due
grain of salt.

You want the API to check its input rigorously, *especially* when they
come from untrusted sources.

If bad input should not happen, you want to abort right away, so you can
debug how it happened.

If the input comes from the guest, you want the API to return an error,
so you can log the guest's bad behavior, and recover appropriately.
Often, the only practical recovery is to put the device in question in a
guest-visible error state, which can be escaped only via device reset.

If the input comes from a monitor command, you also want the API to
return an error, so you can report the error and fail the command.

Could you give a use case for silently doing nothing / returning
garbage?

>> This series is a step towards getting rid of such code,
>
> Sure, incremental patches are good. At this point I think it's
> a good idea to clearly mark this as RFC - I don't think we should yet merge
> this upstream until the solution is a bit more complete.
>
> Changing virtio is the easy part though, so I'm not sure it's a good
> idea to start there.

Does this series hinder work on the harder parts in any way?  Does it
pick a specific solution that may not work for the harder parts?

If not, then I can't see what keeping it out of tree can buy us.

>> if there are trivial
>> ways to abort qemu from a guest bug,
>
> There's no "if" here :)
>
>> then they may be candidates to improve as
>> well.
>
> I'd say let's see some progress on the harder parts of the problem first.
>
>
>> Regarding the malicious guest, protecting D.O.S. attack is also
>> valuable, isn't
>> it?
>> 
>> Thanks,
>> Fam
>
> Guest denying itself a service? I'm not sure why it's valuable.

If I remember correctly, the DOS involved passthrough of a virtual
device to a nested guest or something like that.  Guest killing itself
is unexciting, nested guest killing its host qualifies as DOS.  I guess
our current answer to that is "don't do that then".

> If we can have a guarantee (even if that's only for some specific
> machine types) that illegal input won't ever cause QEMU
> to abort, that could be valuable e.g. to make fuzzing easier.
> Just not having it abort on a specific path only serves to make
> guest driver development easier. That also has value but only for
> things that are hard for drivers to get right.
> E.g. what's the chance driver gets the header size wrong? Probably nil.

Too complicated an argument for me.  I'd rather stick to a simple rule:
if bad input comes from the guest or a monitor command, log / report the
error and recover.

But yes, we need to get there step by step, and some steps are more
important than others.