[v1] Virtio dmabuf improvements

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

Posted by Albert Esteve 1 year ago

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)) {
+    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;
+    }
+
     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

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

Posted by Marc-André Lureau 11 months, 4 weeks ago

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

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

Posted by Albert Esteve 11 months, 3 weeks ago

On Mon, Dec 4, 2023 at 8:54 AM Marc-André Lureau <marcandre.lureau@gmail.com>
wrote:

> 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)) {
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~
>
>
Oops I didn't notice this. Maybe I am missing the
`Werror` flag when I compile locally. I'll fix it.


> > +    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?
>
>
TYPE_DMABUF are meant for virtio devices that run with Qemu
(i.e., not vhost). So owners will not send these messages, but
access the hash table directly.


> > +    }
> > +
> >      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
>
>