Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG skipping mmap_addr

[Stefan Hajnoczi]

On Mon, Oct 11, 2021 at 10:10:47PM +0200, David Hildenbrand wrote:
> We end up not copying the mmap_addr of all existing regions, resulting
> in a SEGFAULT once we actually try to map/access anything within our
> memory regions.
> 
> Fixes: 875b9fd97b34 ("Support individual region unmap in libvhost-user")
> Cc: qemu-stable@nongnu.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Coiby Xu <coiby.xu@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c 
> b/subprojects/libvhost-user/libvhost-user.c
> index bf09693255..787f4d2d4f 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -816,6 +816,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>              shadow_regions[j].gpa = dev->regions[i].gpa;
>              shadow_regions[j].size = dev->regions[i].size;
>              shadow_regions[j].qva = dev->regions[i].qva;
> +            shadow_regions[j].mmap_addr = dev->regions[i].mmap_addr;
>              shadow_regions[j].mmap_offset = dev->regions[i].mmap_offset;
>              j++;
>          } else {

Raphael: Some questions about vu_rem_mem_reg():

- What ensures that shadow_regions[VHOST_USER_MAX_RAM_SLOTS] is large
  enough? The add_mem_reg/set_mem_table code doesn't seem to check
  whether there is enough space in dev->regions[] before adding regions.

- What happens when the master populated dev->regions[] with multiple
  copies of the same region? dev->nregions is only decremented once and
  no longer accurately reflects the number of elements in
  dev->regions[].

libvhost-user must not trust the vhost-user master since vhost-user
needs to provide process isolation. Please add input validation.

Stefan

signature.asc
Description: PGP signature