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

On 29 May 2013 09:24, Michael S. Tsirkin <mst at redhat.com>
wrote:
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f4db224..fd09ea7 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -467,51 +467,70 @@ static uint64_t virtio_pci_config_common_read(void
*opaque, hwaddr addr,
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      VirtIODevice *vdev = proxy->vdev;
> +    struct virtio_pci_common_cfg cfg;
>
>      uint64_t low = 0xffffffffull;
>
>      switch (addr) {
>      case offsetof(struct virtio_pci_common_cfg, device_feature_select):
> +        assert(size == sizeof cfg.device_feature_select);
>          return proxy->device_feature_select;
Asserting is definitely the wrong thing here, since the
guest can trigger it.

If you really want to use offsetof like this you're
going to need to decorate the structs with QEMU_PACKED.

thanks
-- PMM

On Wed, May 29, 2013 at 10:00:33AM +0100, Peter Maydell
wrote:
> On 29 May 2013 09:24, Michael S. Tsirkin <mst at redhat.com> wrote:
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f4db224..fd09ea7 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -467,51 +467,70 @@ static uint64_t
virtio_pci_config_common_read(void *opaque, hwaddr addr,
> >  {
> >      VirtIOPCIProxy *proxy = opaque;
> >      VirtIODevice *vdev = proxy->vdev;
> > +    struct virtio_pci_common_cfg cfg;
> >
> >      uint64_t low = 0xffffffffull;
> >
> >      switch (addr) {
> >      case offsetof(struct virtio_pci_common_cfg,
device_feature_select):
> > +        assert(size == sizeof cfg.device_feature_select);
> >          return proxy->device_feature_select;
> 
> Asserting is definitely the wrong thing here, since the
> guest can trigger it.
So?

It's a driver bug. It can reset or crash guest with the same effect,
and it likely will if we let it continue.
assert makes sure we don't let it escalate into some
hard to debug security problem.

> If you really want to use offsetof like this you're
> going to need to decorate the structs with QEMU_PACKED.
> 
> thanks
> -- PMM
Nope.
These structs are carefully designed not to have any padding.

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.

-- 
MST

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