Re: [PATCH] vhost: fix repeated memory unmap in error paths

[Konstantin Khlebnikov]

10.02.2022, 15:06, "Philippe Mathieu-Daudé" <f4bug@amsat.org>:

On 10/2/22 12:46, Konstantin Khlebnikov wrote:

 Fuzzing found that on some error paths vhost_memory_unmap() is called twice or
 for NULL address. Let's reset pointers after unmap and ingnore unmap for NULL.
 
 Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
 ---
   hw/virtio/vhost.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
 index 7b03efccec..4e5d5f2ea4 100644
 --- a/hw/virtio/vhost.c
 +++ b/hw/virtio/vhost.c
 @@ -335,7 +335,7 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
                                  hwaddr len, int is_write,
                                  hwaddr access_len)
   {
 - if (!vhost_dev_has_iommu(dev)) {
 + if (buffer && !vhost_dev_has_iommu(dev)) {

Shouldn't we simply add an "assert(buffer);" check here instead?

 

Yes, probably error path in queue management should be fixed instead.

But handling NULL pointers in functions like free() make code cleaner.

 

 

           cpu_physical_memory_unmap(buffer, len, is_write, access_len);
       }
   }
 @@ -1191,6 +1191,7 @@ fail_alloc_avail:
       vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
                          0, 0);
   fail_alloc_desc:
 + vq->used = vq->avail = vq->desc = NULL;
       return r;
   }
   
 @@ -1238,6 +1239,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
                          0, virtio_queue_get_avail_size(vdev, idx));
       vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
                          0, virtio_queue_get_desc_size(vdev, idx));
 + vq->used = vq->avail = vq->desc = NULL;
   }

This part is OK.

 

 

-- 

Константин Хлебников

https://staff.yandex-team.ru/khlebnikov