| Message ID | 1575874847-5792-2-git-send-email-raphael.norwitz@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user: Lift Max Ram Slots Limitation | expand |
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.norwitz@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
Makes sense - will fix On Tue, Jan 14, 2020 at 02:07:03AM -0500, Michael S. Tsirkin wrote: > > 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.norwitz@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 > >
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;
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.norwitz@nutanix.com> --- hw/virtio/vhost-user.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)