Re: [PATCH for-5.2] vhost-user: fix VHOST_USER_ADD/REM_MEM_REG truncation

[Cornelia Huck]

On Mon,  9 Nov 2020 17:43:55 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> QEMU currently truncates the mmap_offset field when sending
> VHOST_USER_ADD_MEM_REG and VHOST_USER_REM_MEM_REG messages. The struct
> layout looks like this:
> 
>   typedef struct VhostUserMemoryRegion {
>       uint64_t guest_phys_addr;
>       uint64_t memory_size;
>       uint64_t userspace_addr;
>       uint64_t mmap_offset;
>   } VhostUserMemoryRegion;
> 
>   typedef struct VhostUserMemRegMsg {
>       uint32_t padding;
>       /* WARNING: there is a 32-bit hole here! */
>       VhostUserMemoryRegion region;
>   } VhostUserMemRegMsg;
> 
> The payload size is calculated as follows when sending the message in
> hw/virtio/vhost-user.c:
> 
>   msg->hdr.size = sizeof(msg->payload.mem_reg.padding) +
>       sizeof(VhostUserMemoryRegion);
> 
> This calculation produces an incorrect result of only 36 bytes.
> sizeof(VhostUserMemRegMsg) is actually 40 bytes.
> 
> The consequence of this is that the final field, mmap_offset, is
> truncated. This breaks x86_64 TCG guests on s390 hosts. Other guest/host
> combinations may get lucky if either of the following holds:
> 1. The guest memory layout does not need mmap_offset != 0.
> 2. The host is little-endian and mmap_offset <= 0xffffffff so the
>    truncation has no effect.
> 
> Fix this by extending the existing 32-bit padding field to 64-bit. Now
> the padding reflects the actual compiler padding. This can be verified
> using pahole(1).
> 
> Also document the layout properly in the vhost-user specification.  The
> vhost-user spec did not document the exact layout. It would be
> impossible to implement the spec without looking at the QEMU source
> code.
> 
> Existing vhost-user frontends and device backends continue to work after
> this fix has been applied. The only change in the wire protocol is that
> QEMU now sets hdr.size to 40 instead of 36. If a vhost-user
> implementation has a hardcoded size check for 36 bytes, then it will
> fail with new QEMUs. Both QEMU and DPDK/SPDK don't check the exact
> payload size, so they continue to work.

Seems we are lucky, then.

> 
> Fixes: f1aeb14b0809e313c74244d838645ed25e85ea63 ("Transmit vhost-user memory 
> regions individually")

I think the canonical format is

Fixes: f1aeb14b0809 ("Transmit vhost-user memory regions individually")

Maybe cc:stable as well?


> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/interop/vhost-user.rst           | 21 +++++++++++++++++++--
>  contrib/libvhost-user/libvhost-user.h |  2 +-
>  hw/virtio/vhost-user.c                |  5 ++---
>  3 files changed, 22 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>