Albert Esteve <aest...@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 <aest...@redhat.com>
> Acked-by: Stefan Hajnoczi <stefa...@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
