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

[Albert Esteve <aesteve@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3415 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4902 bytes --]