On Tue, 7 Jun 2022 11:36:58 +0100
Will Deacon <will@xxxxxxxxxx> wrote:
Hi Will,
> On Wed, Jun 01, 2022 at 05:51:36PM +0100, Andre Przywara wrote:
> > The handlers for accessing the virtio-mmio header tried to be very
> > clever, by modelling the internal data structure to look exactly like
> > the protocol header, so that address offsets can "reused".
> >
> > This requires using a packed structure, which creates other problems,
> > and seems to be totally unnecessary in this case.
> >
> > Replace the offset-based access hacks to the structure with proper
> > compiler visible accesses, to avoid unaligned accesses and make the code
> > more robust.
> >
> > This fixes UBSAN complaints about unaligned accesses.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> > include/kvm/virtio-mmio.h | 2 +-
> > virtio/mmio.c | 19 +++++++++++++++----
> > 2 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
> > index 13dcccb6..aa4cab3c 100644
> > --- a/include/kvm/virtio-mmio.h
> > +++ b/include/kvm/virtio-mmio.h
> > @@ -39,7 +39,7 @@ struct virtio_mmio_hdr {
> > u32 interrupt_ack;
> > u32 reserved_5[2];
> > u32 status;
> > -} __attribute__((packed));
> > +};
>
> Does this mean that the previous patch is no longer required?
To some degree patch 1/4 is the quick fix. But I think ordering
struct members in an efficient way is never a bad idea, so that patch
still has some use.
> > struct virtio_mmio {
> > u32 addr;
> > diff --git a/virtio/mmio.c b/virtio/mmio.c
> > index 3782d55a..c9ad8ee7 100644
> > --- a/virtio/mmio.c
> > +++ b/virtio/mmio.c
> > @@ -135,12 +135,22 @@ static void virtio_mmio_config_in(struct kvm_cpu *vcpu,
> >
> > switch (addr) {
> > case VIRTIO_MMIO_MAGIC_VALUE:
> > + memcpy(data, &vmmio->hdr.magic, sizeof(vmmio->hdr.magic));
>
> Hmm, this is a semantic change as we used to treat the magic as a u32 by
> passing it to ioport__write32(), which would in turn do the swab for
> big-endian machines.
Ah, it's big endian testing time again (is it already that time of the
year?)
>
> I don't think we should be using raw memcpy() here.
I will check, thanks for having a look!
Cheers,
Andre
