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

["Alex Bennée" <alex.bennee@linaro.org>]

Albert Esteve <aesteve@redhat.com> writes:

> 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.

Was this buggy behaviour on the part of the vhost-user daemon?

> 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>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/interop/vhost-user.rst |  4 +++-
>  hw/virtio/vhost-user.c      | 21 +++++++++++++++++++--
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 9f1103f85a..60ec2c9d48 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -1839,7 +1839,9 @@ is sent by the front-end.
>    When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
>    feature has been successfully negotiated, this message can be submitted
>    by the backend to remove themselves from to the virtio-dmabuf shared
> -  table API. The shared table will remove the back-end device associated with
> +  table API. Only the back-end owning the entry (i.e., the one that first added
> +  it) will have permission to remove it. Otherwise, the message is ignored.
> +  The shared table will remove the back-end device associated with
>    the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the
>    back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond
>    with zero when operation is successfully completed, or non-zero otherwise.
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index f214df804b..1c3f2357be 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1611,11 +1611,27 @@ 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;
>  
>      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> +    switch (virtio_object_type(&uuid)) {
> +    case TYPE_VHOST_DEV:

It would be nice if we could add a kdoc annotation to SharedObjectType
describing what the various types mean.

> +    {
> +        struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
> +        if (owner == NULL || dev != owner) {

I dev is always set dev != owner should also cover the NULL case.
However will we see uuid's that aren't associated with anything?

> +            /* Not allowed to remove non-owned entries */
> +            return 0;
> +        }
> +        break;
> +    }
> +    default:
> +        /* Not allowed to remove non-owned entries */
> +        return 0;
> +    }
> +
>      return virtio_remove_resource(&uuid);
>  }
>  
> @@ -1794,7 +1810,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,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro