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

[Albert Esteve <aesteve@redhat.com>]

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

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

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