| Message ID | 20240109125614.220293-2-aesteve@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio dmabuf improvements | expand |
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? > 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,
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 > >
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,