[PATCH] Stop vhost-user sending uninitialized mmap_offsets

Raphael Norwitz posted 1 patch 3 years, 9 months ago
Test FreeBSD passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1592637581-25255-1-git-send-email-raphael.norwitz@nutanix.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
hw/virtio/vhost-user.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[PATCH] Stop vhost-user sending uninitialized mmap_offsets
Posted by Raphael Norwitz 3 years, 9 months ago
Prior to this change, the vhost_user_fill_msg_region function filled out
all elements of the VhostUserMemoryRegion struct except the mmap_offset.

This function is often called on uninitialized structs, which are then
copied into VHOST_USER_SET_MEM_TABLE and VHOST_USER_ADD/REM_MEM_REG
messages. In some cases, where the mmap_offset was not needed, it was
left uninitialized, causing QEMU to send the backend uninitialized data,
which Coverity flagged as a series of issues.

This change augments the vhost_user_fill_msg_region API, adding a
mmap_offset paramenter, forcing the caller to initialize mmap_offset.

Fixes: CID-1429802, CID-1429803, CID-1429804

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 hw/virtio/vhost-user.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4d6cd4e..3123121 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -460,12 +460,14 @@ static MemoryRegion *vhost_user_get_mr_data(uint64_t addr, ram_addr_t *offset,
 }
 
 static void vhost_user_fill_msg_region(VhostUserMemoryRegion *dst,
-                                       struct vhost_memory_region *src)
+                                       struct vhost_memory_region *src,
+                                       uint64_t mmap_offset)
 {
     assert(src != NULL && dst != NULL);
     dst->userspace_addr = src->userspace_addr;
     dst->memory_size = src->memory_size;
     dst->guest_phys_addr = src->guest_phys_addr;
+    dst->mmap_offset = mmap_offset;
 }
 
 static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u,
@@ -500,9 +502,8 @@ static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u,
                 error_report("Failed preparing vhost-user memory table msg");
                 return -1;
             }
-            vhost_user_fill_msg_region(&region_buffer, reg);
+            vhost_user_fill_msg_region(&region_buffer, reg, offset);
             msg->payload.memory.regions[*fd_num] = region_buffer;
-            msg->payload.memory.regions[*fd_num].mmap_offset = offset;
             fds[(*fd_num)++] = fd;
         } else if (track_ramblocks) {
             u->region_rb_offset[i] = 0;
@@ -649,7 +650,7 @@ static int send_remove_regions(struct vhost_dev *dev,
 
         if (fd > 0) {
             msg->hdr.request = VHOST_USER_REM_MEM_REG;
-            vhost_user_fill_msg_region(&region_buffer, shadow_reg);
+            vhost_user_fill_msg_region(&region_buffer, shadow_reg, 0);
             msg->payload.mem_reg.region = region_buffer;
 
             if (vhost_user_write(dev, msg, &fd, 1) < 0) {
@@ -709,9 +710,8 @@ static int send_add_regions(struct vhost_dev *dev,
                 u->region_rb[reg_idx] = mr->ram_block;
             }
             msg->hdr.request = VHOST_USER_ADD_MEM_REG;
-            vhost_user_fill_msg_region(&region_buffer, reg);
+            vhost_user_fill_msg_region(&region_buffer, reg, offset);
             msg->payload.mem_reg.region = region_buffer;
-            msg->payload.mem_reg.region.mmap_offset = offset;
 
             if (vhost_user_write(dev, msg, &fd, 1) < 0) {
                 return -1;
-- 
1.8.3.1


Re: [PATCH] Stop vhost-user sending uninitialized mmap_offsets
Posted by Michael S. Tsirkin 3 years, 9 months ago
On Mon, Jun 22, 2020 at 06:41:39PM +0000, Raphael Norwitz wrote:
> Prior to this change, the vhost_user_fill_msg_region function filled out
> all elements of the VhostUserMemoryRegion struct except the mmap_offset.
> 
> This function is often called on uninitialized structs, which are then
> copied into VHOST_USER_SET_MEM_TABLE and VHOST_USER_ADD/REM_MEM_REG
> messages. In some cases, where the mmap_offset was not needed, it was
> left uninitialized, causing QEMU to send the backend uninitialized data,
> which Coverity flagged as a series of issues.
> 
> This change augments the vhost_user_fill_msg_region API, adding a
> mmap_offset paramenter, forcing the caller to initialize mmap_offset.
> 
> Fixes: CID-1429802, CID-1429803, CID-1429804


Add commit hash of the buggy commit pls.

> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  hw/virtio/vhost-user.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 4d6cd4e..3123121 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -460,12 +460,14 @@ static MemoryRegion *vhost_user_get_mr_data(uint64_t addr, ram_addr_t *offset,
>  }
>  
>  static void vhost_user_fill_msg_region(VhostUserMemoryRegion *dst,
> -                                       struct vhost_memory_region *src)
> +                                       struct vhost_memory_region *src,
> +                                       uint64_t mmap_offset)
>  {
>      assert(src != NULL && dst != NULL);
>      dst->userspace_addr = src->userspace_addr;
>      dst->memory_size = src->memory_size;
>      dst->guest_phys_addr = src->guest_phys_addr;
> +    dst->mmap_offset = mmap_offset;
>  }
>  
>  static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u,
> @@ -500,9 +502,8 @@ static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u,
>                  error_report("Failed preparing vhost-user memory table msg");
>                  return -1;
>              }
> -            vhost_user_fill_msg_region(&region_buffer, reg);
> +            vhost_user_fill_msg_region(&region_buffer, reg, offset);
>              msg->payload.memory.regions[*fd_num] = region_buffer;
> -            msg->payload.memory.regions[*fd_num].mmap_offset = offset;
>              fds[(*fd_num)++] = fd;
>          } else if (track_ramblocks) {
>              u->region_rb_offset[i] = 0;
> @@ -649,7 +650,7 @@ static int send_remove_regions(struct vhost_dev *dev,
>  
>          if (fd > 0) {
>              msg->hdr.request = VHOST_USER_REM_MEM_REG;
> -            vhost_user_fill_msg_region(&region_buffer, shadow_reg);
> +            vhost_user_fill_msg_region(&region_buffer, shadow_reg, 0);
>              msg->payload.mem_reg.region = region_buffer;
>  
>              if (vhost_user_write(dev, msg, &fd, 1) < 0) {
> @@ -709,9 +710,8 @@ static int send_add_regions(struct vhost_dev *dev,
>                  u->region_rb[reg_idx] = mr->ram_block;
>              }
>              msg->hdr.request = VHOST_USER_ADD_MEM_REG;
> -            vhost_user_fill_msg_region(&region_buffer, reg);
> +            vhost_user_fill_msg_region(&region_buffer, reg, offset);
>              msg->payload.mem_reg.region = region_buffer;
> -            msg->payload.mem_reg.region.mmap_offset = offset;
>  
>              if (vhost_user_write(dev, msg, &fd, 1) < 0) {
>                  return -1;
> -- 
> 1.8.3.1