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

[Raphael Norwitz <raphael.norwitz@nutanix.com>]

On Mon, Oct 18, 2021 at 09:49:53AM -0400, Michael S. Tsirkin wrote:
> On Thu, Oct 14, 2021 at 04:52:48AM +0000, Raphael Norwitz wrote:
> > On Wed, Oct 13, 2021 at 10:40:46AM +0100, Stefan Hajnoczi wrote:
> > > 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.
> > >
> > 
> > Correct - it does not check if there is enough space as is. I can add that.
> 
> 
> Just making sure - you are now working on series supreceding this patch?
> Is that right?

I was just going to fix the missing input validation. This looks like a
standalone issue and in my opinon the fix should go in as is. I will
base my changes on top of it. 

> 
> > > - 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[].
> > 
> > That case is also not accounted for. I will add it.
> > 
> > > 
> > > libvhost-user must not trust the vhost-user master since vhost-user
> > > needs to provide process isolation. Please add input validation.
> > > 
> > 
> > Got it - let me start working on a series.
> > 
> > > Stefan
>