| Message ID | 1594799958-31356-1-git-send-email-raphael.norwitz@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix vhost-user buffer over-read on ram hot-unplug | expand |
ping On Thu, Jul 16, 2020 at 10:21 PM Raphael Norwitz <raphael.norwitz@nutanix.com> wrote: > > The VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS vhost-user protocol > feature introduced a shadow-table, used by the backend to dynamically > determine how a vdev's memory regions have changed since the last > vhost_user_set_mem_table() call. On hot-remove, a memmove() operation > is used to overwrite the removed shadow region descriptor(s). The size > parameter of this memmove was off by 1 such that if a VM with a backend > supporting the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS filled it's > shadow-table (by performing the maximum number of supported hot-add > operatons) and attempted to remove the last region, Qemu would read an > out of bounds value and potentially crash. > > This change fixes the memmove() bounds such that this erroneous read can > never happen. > > Signed-off-by: Peter Turschmid <peter.turschm@nutanix.com> > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > hw/virtio/vhost-user.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 3123121..d7e2423 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -672,7 +672,7 @@ static int send_remove_regions(struct vhost_dev *dev, > memmove(&u->shadow_regions[shadow_reg_idx], > &u->shadow_regions[shadow_reg_idx + 1], > sizeof(struct vhost_memory_region) * > - (u->num_shadow_regions - shadow_reg_idx)); > + (u->num_shadow_regions - shadow_reg_idx - 1)); > u->num_shadow_regions--; > } > > -- > 1.8.3.1 > >
On Fri, Jul 17, 2020 at 8:21 AM Raphael Norwitz <raphael.norwitz@nutanix.com> wrote: > > The VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS vhost-user protocol > feature introduced a shadow-table, used by the backend to dynamically > determine how a vdev's memory regions have changed since the last > vhost_user_set_mem_table() call. On hot-remove, a memmove() operation > is used to overwrite the removed shadow region descriptor(s). The size > parameter of this memmove was off by 1 such that if a VM with a backend > supporting the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS filled it's > shadow-table (by performing the maximum number of supported hot-add > operatons) and attempted to remove the last region, Qemu would read an > out of bounds value and potentially crash. > > This change fixes the memmove() bounds such that this erroneous read can > never happen. > > Signed-off-by: Peter Turschmid <peter.turschm@nutanix.com> > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> Fixes: f1aeb14b0809 ("Transmit vhost-user memory regions individually") Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/virtio/vhost-user.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 3123121..d7e2423 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -672,7 +672,7 @@ static int send_remove_regions(struct vhost_dev *dev, > memmove(&u->shadow_regions[shadow_reg_idx], > &u->shadow_regions[shadow_reg_idx + 1], > sizeof(struct vhost_memory_region) * > - (u->num_shadow_regions - shadow_reg_idx)); > + (u->num_shadow_regions - shadow_reg_idx - 1)); > u->num_shadow_regions--; > } > > -- > 1.8.3.1 >
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 3123121..d7e2423 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -672,7 +672,7 @@ static int send_remove_regions(struct vhost_dev *dev, memmove(&u->shadow_regions[shadow_reg_idx], &u->shadow_regions[shadow_reg_idx + 1], sizeof(struct vhost_memory_region) * - (u->num_shadow_regions - shadow_reg_idx)); + (u->num_shadow_regions - shadow_reg_idx - 1)); u->num_shadow_regions--; }