Re: [PATCH RFC] virtio-pci: new config layout: using memory BAR

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 29, 2013 at 10:00:33AM +0100, Peter Maydell wrote:
> On 29 May 2013 09:24, Michael S. Tsirkin <mst@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