Re: [PATCH 1/3] hw/virtio: check owner for removing objects

[Marc-André Lureau]

On Tue, Nov 7, 2023 at 1:37 PM Albert Esteve <aesteve@redhat.com> wrote:
>
> Shared objects lack spoofing protection.
> For VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE messages
> received by the vhost-user interface, any backend was
> allowed to remove entries from the shared table just
> by knowing the UUID. Only the owner of the entry
> shall be allowed to removed their resources
> from the table.
>
> To fix that, add a check for all
> *SHARED_OBJECT_REMOVE messages received.
> A vhost device can only remove TYPE_VHOST_DEV
> entries that are owned by them, otherwise skip
> the removal, and inform the device that the entry
> has not been removed in the answer.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  hw/virtio/vhost-user.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 7b42ae8aae..5fdff0241f 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1602,10 +1602,26 @@ vhost_user_backend_handle_shared_object_add(struct 
> vhost_dev *dev,
>  }
>
>  static int
> -vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
> +vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
> +                                               VhostUserShared *object)
>  {
>      QemuUUID uuid;
>
> +    switch (virtio_object_type(&uuid)) {

../hw/virtio/vhost-user.c:1619:13: error: ‘uuid’ may be used
uninitialized [-Werror=maybe-uninitialized]
 1619 |     switch (virtio_object_type(&uuid)) {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~

> +    case TYPE_VHOST_DEV:
> +    {
> +        struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
> +        if (owner == NULL || dev != owner) {
> +            /* Not allowed to remove non-owned entries */
> +            return 0;
> +        }
> +        break;
> +    }
> +    default:
> +        /* Not allowed to remove non-owned entries */
> +        return 0;

How do you remove TYPE_DMABUF entries after this patch?

> +    }
> +
>      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
>      return virtio_remove_resource(&uuid);
>  }
> @@ -1785,7 +1801,8 @@ static gboolean backend_read(QIOChannel *ioc, 
> GIOCondition condition,
>          ret = vhost_user_backend_handle_shared_object_add(dev, 
> &payload.object);
>          break;
>      case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
> -        ret = 
> vhost_user_backend_handle_shared_object_remove(&payload.object);
> +        ret = vhost_user_backend_handle_shared_object_remove(dev,
> +                                                             
> &payload.object);
>          break;
>      case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
>          ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, 
> ioc,
> --
> 2.41.0
>


-- 
Marc-André Lureau