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>
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:
+ {
+ 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;
+ }
+
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,
--
2.43.0Albert 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
On Mon, Feb 5, 2024 at 1:57 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> 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?
>
Yes, although the feature is not really used yet, and it requires to know
the UUID to be able to exploit it. But yes, any vhost-user backend could
remove any entry.
>
> > 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.
>
I can add it.
>
> > + {
> > + 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.
>
True, I can remove the NULL case from the condition.
> However will we see uuid's that aren't associated with anything?
>
Theoretically, it shouldn't happen. Dmabufs in the host and the guest are
aligned,
and when one buffer is cleaned up it should not be requested anymore, as
the drivers
in the guest are aware. But a vhost-user backend could have buggy/malicious
requests, so worth the check.
>
> > + /* 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
>
>
© 2016 - 2024 Red Hat, Inc.