Virtualization - May 2013 - [PATCH RFC] virtio-pci: new config layout: using memory BAR

On 29 May 2013 11:08, Michael S. Tsirkin <mst at redhat.com>
wrote:
> On Wed, May 29, 2013 at 10:00:33AM +0100, Peter Maydell wrote:
>> Asserting is definitely the wrong thing here, since the
>> guest can trigger it.
>
> So?
So "guest should not be able to crash QEMU" is a standard
rule: assert() is for QEMU bugs, not guest bugs.
Virtio isn't any different to any other device model.
> It's a driver bug. It can reset or crash guest with the same effect,
> and it likely will if we let it continue.
Letting a misbehaving guest crash itself is fine. Killing
QEMU isn't.
> assert makes sure we don't let it escalate into some
> hard to debug security problem.
If you want to make guest bugs easier to spot and debug
this is what qemu_log_mask(LOG_GUEST_ERROR,...) is for.
>> If you really want to use offsetof like this you're
>> going to need to decorate the structs with QEMU_PACKED.
> Nope.
> These structs are carefully designed not to have any padding.
...on every compiler and OS combination that QEMU builds for?
> And if there was a bug and there was some padding, we still
> can't fix it with PACKED because this structure
> is used to interact with the guest code which does not
> have the packed attribute.
The guest code has to use a set of structure offsets and
sizes which is fixed by the virtio ABI -- how it implements
this is up to the guest (and if it misimplements it that is
a guest bug and not our problem). Note also that the ABI
is not defined by a set of C source struct definitions
(except in as far as they are accompanied by a set of
restrictions on alignment, padding, etc that completely
determine the numerical alignments and offsets).
How QEMU implements the set of offsets and sizes specified
by the ABI is definitely our problem, and is exactly what
this discussion is about. The simplest and safest way to
get the offsets right on all platforms is just to use a set
of #defines, which is why that's what almost all of our
device models do. Where we do define a struct matching
guest memory layout, we tag it with QEMU_PACKED as our
best means of ensuring consistency on all hosts.

thanks
-- PMM

On Wed, May 29, 2013 at 11:53:17AM +0100, Peter Maydell
wrote:
> On 29 May 2013 11:08, Michael S. Tsirkin <mst at redhat.com> wrote:
> > On Wed, May 29, 2013 at 10:00:33AM +0100, Peter Maydell wrote:
> >> Asserting is definitely the wrong thing here, since the
> >> guest can trigger it.
> >
> > So?
> 
> So "guest should not be able to crash QEMU" is a standard
> rule: assert() is for QEMU bugs, not guest bugs.
It's quite likely a QEMU bug -
guests normally do fixed-size accesses so it's hard
for them to access a field with a wrong size.

It is guest triggerable but this does not
contradict the fact that it never happens in practice.
> Virtio isn't any different to any other device model.
> 
> > It's a driver bug. It can reset or crash guest with the same
effect,
> > and it likely will if we let it continue.
> 
> Letting a misbehaving guest crash itself is fine. Killing
> QEMU isn't.
*Why* isn't it?
It has the advantage of making sure the misbehaving system
is stopped before it does any damage.
Why keep QEMU running even though we know
there's a memory corruption somewhere?
> > assert makes sure we don't let it escalate into some
> > hard to debug security problem.
> 
> If you want to make guest bugs easier to spot and debug
> this is what qemu_log_mask(LOG_GUEST_ERROR,...) is for.
We really want something that would also stop guest
and stop device model as well - we don't know where the bug is.
> >> If you really want to use offsetof like this you're
> >> going to need to decorate the structs with QEMU_PACKED.
> 
> > Nope.
> > These structs are carefully designed not to have any padding.
> 
> ...on every compiler and OS combination that QEMU builds for?
Yes. All the way back to EGCS and before.
GCC has been like this for many many years.
> > And if there was a bug and there was some padding, we still
> > can't fix it with PACKED because this structure
> > is used to interact with the guest code which does not
> > have the packed attribute.
> 
> The guest code has to use a set of structure offsets and
> sizes which is fixed by the virtio ABI -- how it implements
> this is up to the guest (and if it misimplements it that is
> a guest bug and not our problem). Note also that the ABI
> is not defined by a set of C source struct definitions
> (except in as far as they are accompanied by a set of
> restrictions on alignment, padding, etc that completely
> determine the numerical alignments and offsets).
In practical terms, we should all talk and agree on what's
best for driver *and* QEMU, not have QEMU just
ignore driver and do it's own thing.

In practical terms, virtio in QEMU should use
exactly same code to define layout as virtio in guest.
Preferably same header file, we'll get there too,
once we comple the discussion over the bikeshed color.

Deviating from driver in random ways is an endless source
of hard to debug issues, and it's a very practical
consideration because virtio spec is constantly
being extended (unlike hardware which is mostly fixed).

> How QEMU implements the set of offsets and sizes specified
> by the ABI is definitely our problem, and is exactly what
> this discussion is about. The simplest and safest way to
> get the offsets right on all platforms is just to use a set
> of #defines, which is why that's what almost all of our
> device models do.
Maybe you don't feel safe when you see offsetof.
I review a struct and see fields are aligned
properly at a glance and I feel safe.

But I don't feel safe when I see headers in guest and qemu have
different code.
> Where we do define a struct matching
> guest memory layout, we tag it with QEMU_PACKED as our
> best means of ensuring consistency on all hosts.
> 
> thanks
> -- PMM
Further, packed structures produce terrible
code in GCC, in all versions that QEMU cares about :).

-- 
MST

Il 29/05/2013 14:16, Michael S. Tsirkin ha scritto:
>>>> > >> If you really want to use offsetof like this
you're
>>>> > >> going to need to decorate the structs with
QEMU_PACKED.
>> > 
>>> > > Nope.
>>> > > These structs are carefully designed not to have any
padding.
>> > 
>> > ...on every compiler and OS combination that QEMU builds for?
> Yes. All the way back to EGCS and before.
> GCC has been like this for many many years.
I would still prefer to have QEMU_PACKED (or __attribute((__packed__))
in the kernel).
>>> > > And if there was a bug and there was some padding, we
still
>>> > > can't fix it with PACKED because this structure
>>> > > is used to interact with the guest code which does not
>>> > > have the packed attribute.
>> > 
>> > The guest code has to use a set of structure offsets and
>> > sizes which is fixed by the virtio ABI -- how it implements
>> > this is up to the guest (and if it misimplements it that is
>> > a guest bug and not our problem).
On the other hand, encouraging both userspace and guest to reuse a
single set of headers means that the bad offset becomes a spec bug more
than a guest bug.
> Deviating from driver in random ways is an endless source
> of hard to debug issues, and it's a very practical
> consideration because virtio spec is constantly
> being extended (unlike hardware which is mostly fixed).
Agreed---but the driver should use __attribute__((__packed__)) too.

Paolo