On Mon, Dec 4, 2023 at 8:54 AM Marc-André Lureau <marcandre.lur...@gmail.com>
wrote:
> On Tue, Nov 7, 2023 at 1:37 PM Albert Esteve <aest...@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 <aest...@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
>
>
