On Thu, Sep 12, 2024 at 04:53:32PM +0200, Albert Esteve wrote:
> Update shmem_list to be able to track
> active mappings on VIRTIO shared memory
> regions. This allows to verify that new
> mapping request received from backends
> do not overlap. If they do, the request
> shall fail in order to adhere to the specs.
>
> Signed-off-by: Albert Esteve <aest...@redhat.com>
> ---
> hw/virtio/vhost-user.c | 31 +++++++++++++-------
> hw/virtio/virtio.c | 58 ++++++++++++++++++++++++++++++++++----
> include/hw/virtio/virtio.h | 25 ++++++++++++++--
> 3 files changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 338cc942ec..de0bb35257 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1776,7 +1776,7 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev
> *dev,
> int fd)
> {
> void *addr = 0;
> - MemoryRegion *mr = NULL;
> + VirtSharedMemory *shmem = NULL;
>
> if (fd < 0) {
> error_report("Bad fd for map");
> @@ -1791,22 +1791,29 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev
> *dev,
> return -EFAULT;
> }
>
> - mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
>
> - if (!mr) {
> + if (!shmem) {
> error_report("VIRTIO Shared Memory Region at "
> "ID %d unitialized", vu_mmap->shmid);
> return -EFAULT;
> }
>
> if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> - (vu_mmap->shm_offset + vu_mmap->len) > mr->size) {
> + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> vu_mmap->shm_offset, vu_mmap->len);
> return -EFAULT;
> }
>
> - void *shmem_ptr = memory_region_get_ram_ptr(mr);
> + if (virtio_shmem_map_overlaps(shmem, vu_mmap->shm_offset, vu_mmap->len))
> {
> + error_report("Requested memory (%" PRIx64 "+%" PRIx64 ") overalps "
> + "with previously mapped memory",
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
>
> addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> @@ -1818,6 +1825,8 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev
> *dev,
> return -EFAULT;
> }
>
> + virtio_add_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
> return 0;
> }
>
> @@ -1826,7 +1835,7 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev
> *dev,
> VhostUserMMap *vu_mmap)
> {
> void *addr = 0;
> - MemoryRegion *mr = NULL;
> + VirtSharedMemory *shmem = NULL;
>
> if (!dev->vdev->shmem_list ||
> dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> @@ -1836,22 +1845,22 @@ vhost_user_backend_handle_shmem_unmap(struct
> vhost_dev *dev,
> return -EFAULT;
> }
>
> - mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
>
> - if (!mr) {
> + if (!shmem) {
> error_report("VIRTIO Shared Memory Region at "
> "ID %d unitialized", vu_mmap->shmid);
> return -EFAULT;
> }
>
> if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> - (vu_mmap->shm_offset + vu_mmap->len) > mr->size) {
> + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> vu_mmap->shm_offset, vu_mmap->len);
> return -EFAULT;
> }
Please add a check for an existing mapping that matches [shm_offset,
shm_offset + len). This will prevent partial unmap as required by the
spec.
>
> - void *shmem_ptr = memory_region_get_ram_ptr(mr);
> + void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
>
> addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> @@ -1861,6 +1870,8 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev
> *dev,
> return -EFAULT;
> }
>
> + virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
> return 0;
> }
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ccc4f2cd75..0e2cd62a15 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3059,15 +3059,52 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> }
>
> -MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
> {
> - MemoryRegion *mr;
> + VirtSharedMemory *shmem = NULL;
> ++vdev->n_shmem_regions;
> - vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
> + vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
> vdev->n_shmem_regions);
> - mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> - mr = g_new0(MemoryRegion, 1);
> - return mr;
> + shmem = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> + shmem = g_new0(VirtSharedMemory, 1);
> + QTAILQ_INIT(&shmem->mapped_regions);
> + return shmem;
> +}
> +
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
> + mmap->offset = offset;
> + mmap->size = int128_make64(size);
> + QTAILQ_REMOVE(&shmem->mapped_regions, mmap, link);
> + g_free(mmap);
mmap needs to be inserted into mapped_regions and must not be freed by
this function.
> +}
> +
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
> + mmap->offset = offset;
> + mmap->size = int128_make64(size);
> + QTAILQ_INSERT_TAIL(&shmem->mapped_regions, mmap, link);
This function needs to search the list for the matching mmap and remove
it from the list. It should not create a new mmap.
> + g_free(mmap);
> +}
> +
> +bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *map_reg;
> + hwaddr new_reg_end = offset + size;
> + QTAILQ_FOREACH(map_reg, &shmem->mapped_regions, link) {
> + hwaddr region_end = map_reg->offset + map_reg->size;
> + if ((map_reg->offset == offset) ||
> + (map_reg->offset < offset && region_end >= offset) ||
> + (offset < map_reg->offset && new_reg_end >= map_reg->offset )) {
> + return true;
> + }
> + }
> + return false;
> }
>
> /* A wrapper for use as a VMState .put function */
> @@ -4007,11 +4044,20 @@ static void
> virtio_device_free_virtqueues(VirtIODevice *vdev)
> static void virtio_device_instance_finalize(Object *obj)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(obj);
> + VirtSharedMemory *shmem = NULL;
> + int i;
>
> virtio_device_free_virtqueues(vdev);
>
> g_free(vdev->config);
> g_free(vdev->vector_queues);
> + for (i = 0; i< vdev->n_shmem_regions; i++) {
> + shmem = &vdev->shmem_list[i];
> + while (!QTAILQ_EMPTY(&shmem->mapped_regions)) {
> + MappedMemoryRegion *mmap_reg =
> QTAILQ_FIRST(&shmem->mapped_regions);
> + QTAILQ_REMOVE(&shmem->mapped_regions, mmap_reg, link);
g_free(mmap_reg)
> + }
> + }
> }
>
> static Property virtio_properties[] = {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index d4a2f664d9..5b801f33f5 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -98,6 +98,21 @@ enum virtio_device_endian {
> VIRTIO_DEVICE_ENDIAN_BIG,
> };
>
> +struct MappedMemoryRegion {
> + Int128 size;
> + hwaddr offset;
> + QTAILQ_ENTRY(MappedMemoryRegion) link;
> +};
> +
> +typedef struct MappedMemoryRegion MappedMemoryRegion;
> +
> +struct VirtSharedMemory {
> + MemoryRegion *mr;
> + QTAILQ_HEAD(, MappedMemoryRegion) mapped_regions;
> +};
> +
> +typedef struct VirtSharedMemory VirtSharedMemory;
> +
> /**
> * struct VirtIODevice - common VirtIO structure
> * @name: name of the device
> @@ -168,7 +183,7 @@ struct VirtIODevice
> EventNotifier config_notifier;
> bool device_iotlb_enabled;
> /* Shared memory region for vhost-user mappings. */
> - MemoryRegion *shmem_list;
> + VirtSharedMemory *shmem_list;
> int n_shmem_regions;
> };
>
> @@ -289,7 +304,13 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>
> int virtio_save(VirtIODevice *vdev, QEMUFile *f);
>
> -MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
> +bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
>
> extern const VMStateInfo virtio_vmstate_info;
>
> --
> 2.45.2
>
signature.asc
Description: PGP signature
