| Message ID | 20211011201047.62587-1-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] libvhost-user: fix VHOST_USER_REM_MEM_REG skipping mmap_addr | expand |
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> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.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 { > -- > 2.31.1 >
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
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(+) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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. > - 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
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. > > > - 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. Great, thank you! Stefan
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? > > - 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
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 >
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 {
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(+)