On Mon, Dec 09, 2019 at 02:00:45AM -0500, Raphael Norwitz wrote: > The current vhost_user_set_mem_table_postcopy() implementation > populates each region of the VHOST_USER_SET_MEM_TABLE > message without first checking if there are more than > VHOST_MEMORY_MAX_NREGIONS already populated. This can > cause memory corruption and potentially a crash if too many > regions are added to the message during the postcopy step. > > Additionally, after populating each region, the current > implementation asserts that the current region index is less than > VHOST_MEMORY_MAX_NREGIONS. Thus, even if the aforementioned > bug is fixed by moving the existing assert up, too many hot-adds > during the postcopy step will bring down qemu instead of > gracefully propogating up the error as in > vhost_user_set_mem_table(). > > This change cleans up error handling in > vhost_user_set_mem_table_postcopy() such that it handles > an unsupported number of memory hot-adds like > vhost_user_set_mem_table(), gracefully propogating an error > up instead of corrupting memory and crashing qemu. > > Signed-off-by: Raphael Norwitz <raphael.norw...@nutanix.com>
Unfortunately all the nice error handling is mostly futile since vhost_commit does: r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); if (r < 0) { VHOST_OPS_DEBUG("vhost_set_mem_table failed"); } goto out; so the reason there's an assert is that we really must never hit this path at all. So moving assert up seems cleaner. > --- > hw/virtio/vhost-user.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 02a9b25..f74ff3b 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -441,6 +441,10 @@ static int vhost_user_set_mem_table_postcopy(struct > vhost_dev *dev, > &offset); > fd = memory_region_get_fd(mr); > if (fd > 0) { > + if (fd_num == VHOST_MEMORY_MAX_NREGIONS) { > + error_report("Failed preparing vhost-user memory table msg"); > + return -1; > + } > trace_vhost_user_set_mem_table_withfd(fd_num, mr->name, > reg->memory_size, > reg->guest_phys_addr, > @@ -453,7 +457,6 @@ static int vhost_user_set_mem_table_postcopy(struct > vhost_dev *dev, > msg.payload.memory.regions[fd_num].guest_phys_addr = > reg->guest_phys_addr; > msg.payload.memory.regions[fd_num].mmap_offset = offset; > - assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); > fds[fd_num++] = fd; > } else { > u->region_rb_offset[i] = 0; > -- > 1.8.3.1