From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 33C9EC4829E for ; Thu, 15 Feb 2024 09:38:06 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1raYBP-00085P-Vt; Thu, 15 Feb 2024 04:37:32 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1raYBO-00085G-HC for qemu-devel@nongnu.org; Thu, 15 Feb 2024 04:37:30 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1raYBM-00069A-JH for qemu-devel@nongnu.org; Thu, 15 Feb 2024 04:37:30 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1707989847; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Gpd4lECPFg6xf2HFCyQKksU/50mjjHc6aADDlYkeaUs=; b=ANFbkz+IloMyl6FElx1kWcuB/KyJNRb2BaofV8O13f9lWh5B8Suk6rxmFopRXT0Uj889MJ 8EQEdHbXbVz88Cs0bMsZ1wu6qUcmjk+SFcIBOlG/2FJxe2PHMDgR/YmdKzQrcOGir5SoLO nDhjYByznl+riC7uLCCiPHAT6csDZOo= Received: from mail-pj1-f69.google.com (mail-pj1-f69.google.com [209.85.216.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-491-Q7-uEGayPEqfoDrxVV7xzg-1; Thu, 15 Feb 2024 04:37:25 -0500 X-MC-Unique: Q7-uEGayPEqfoDrxVV7xzg-1 Received: by mail-pj1-f69.google.com with SMTP id 98e67ed59e1d1-29608f00cbbso556444a91.2 for ; Thu, 15 Feb 2024 01:37:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707989844; x=1708594644; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Gpd4lECPFg6xf2HFCyQKksU/50mjjHc6aADDlYkeaUs=; b=KWLH4WggLpTH4OjYIr4Fz+SZl98ZfUfFYtrJFCJl5MwObE30PHrV6oEBf2n4e/4lsp lKBwLITRDUfFYZ0k1aL4H796ohW/5WbUkBebTggKXK04hZMhqz6EXabWs1JeKKgOiyWB WnVu1pjzo4VHmoQkxWf+7b2OO6YhdCJuQcJvMuqHc5h0W7Z1iYKdSqxWxZau53Af9hcx ids0u6ONbZFUpNeo4F86KIZHX18czCYvsLzfMWqDLbndX1DZ8MF8KOy+ZzxWntueNwhj TslRRfoxrYgZYY6xfaFfJhg7FQjHP+JYljaNCoKE4eVkZOmFzXp5rKaOqkdWxOT/GmxW kxMQ== X-Gm-Message-State: AOJu0Yw9mnhEeC0hXfCxXvltRYkkHa5FFqZt9QB1f5xLhUxMWyBxy7L5 kmokqgcdFJg4Xc01i5zAxk0I4KAld0EDWzt2w3kbEgh4cFm+3jRf0TS/izrGXhJ0hlSU75B/cPH zLcW3elvu+9t/zqpyvGRh2NVnjibWBcAk84Gd2qDTkMZRgGfDD3nHqqrxpUR5HPSF9TMHDzIKqP 4bE1YgIEB8hKdRsjgF2wA/b8Iv+kY= X-Received: by 2002:a17:90b:10e:b0:297:22a3:43f2 with SMTP id p14-20020a17090b010e00b0029722a343f2mr1064944pjz.41.1707989844383; Thu, 15 Feb 2024 01:37:24 -0800 (PST) X-Google-Smtp-Source: AGHT+IGjkhYBDvsa/o5k2H85fUe/dEOwZCwcUuabyH4HhQAbuuRGEgsFcEdaGBUAS4y4D2U/gpzPO7n9tVMMVaJEXoc= X-Received: by 2002:a17:90b:10e:b0:297:22a3:43f2 with SMTP id p14-20020a17090b010e00b0029722a343f2mr1064935pjz.41.1707989844127; Thu, 15 Feb 2024 01:37:24 -0800 (PST) MIME-Version: 1.0 References: <20240109125614.220293-1-aesteve@redhat.com> <20240109125614.220293-2-aesteve@redhat.com> <87a5off4qe.fsf@draig.linaro.org> In-Reply-To: <87a5off4qe.fsf@draig.linaro.org> From: Albert Esteve Date: Thu, 15 Feb 2024 10:37:12 +0100 Message-ID: Subject: Re: [PATCH v3 1/3] hw/virtio: check owner for removing objects To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: qemu-devel@nongnu.org, stefanha@gmail.com, "Michael S. Tsirkin" , marcandre.lureau@gmail.com, kraxel@redhat.com, Stefan Hajnoczi Content-Type: multipart/alternative; boundary="000000000000f084990611685fb8" Received-SPF: pass client-ip=170.10.129.124; envelope-from=aesteve@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -25 X-Spam_score: -2.6 X-Spam_bar: -- X-Spam_report: (-2.6 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.531, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --000000000000f084990611685fb8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Feb 5, 2024 at 1:57=E2=80=AFPM Alex Benn=C3=A9e wrote: > Albert Esteve 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 > > Acked-by: Stefan Hajnoczi > > --- > > 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 =3D virtio_lookup_vhost_device(&uuid); > > + if (owner =3D=3D NULL || dev !=3D owner) { > > I dev is always set dev !=3D 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 =3D vhost_user_backend_handle_shared_object_add(dev, > &payload.object); > > break; > > case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE: > > - ret =3D > vhost_user_backend_handle_shared_object_remove(&payload.object); > > + ret =3D vhost_user_backend_handle_shared_object_remove(dev, > > + > &payload.object); > > break; > > case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP: > > ret =3D > vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc, > > -- > Alex Benn=C3=A9e > Virtualisation Tech Lead @ Linaro > > --000000000000f084990611685fb8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable



On Mon, Feb 5, 2024 at 1:= 57=E2=80=AFPM Alex Benn=C3=A9e <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=C2=A0the feature is not really used y= et, and it requires to know
the UUID to be able to exploit it. Bu= t yes, any vhost-user backend could
remove any entry.
= =C2=A0

> 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>
> ---
>=C2=A0 docs/interop/vhost-user.rst |=C2=A0 4 +++-
>=C2=A0 hw/virtio/vhost-user.c=C2=A0 =C2=A0 =C2=A0 | 21 ++++++++++++++++= +++--
>=C2=A0 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.
>=C2=A0 =C2=A0 When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol=
>=C2=A0 =C2=A0 feature has been successfully negotiated, this message ca= n be submitted
>=C2=A0 =C2=A0 by the backend to remove themselves from to the virtio-dm= abuf shared
> -=C2=A0 table API. The shared table will remove the back-end device as= sociated with
> +=C2=A0 table API. Only the back-end owning the entry (i.e., the one t= hat first added
> +=C2=A0 it) will have permission to remove it. Otherwise, the message = is ignored.
> +=C2=A0 The shared table will remove the back-end device associated wi= th
>=C2=A0 =C2=A0 the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negot= iated, and the
>=C2=A0 =C2=A0 back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the fro= nt-end must respond
>=C2=A0 =C2=A0 with zero when operation is successfully completed, or no= n-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(st= ruct vhost_dev *dev,
>=C2=A0 }
>=C2=A0
>=C2=A0 static int
> -vhost_user_backend_handle_shared_object_remove(VhostUserShared *objec= t)
> +vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0VhostUserShared *object)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 QemuUUID uuid;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 memcpy(uuid.data, object->uuid, sizeof(object-&= gt;uuid));
> +=C2=A0 =C2=A0 switch (virtio_object_type(&uuid)) {
> +=C2=A0 =C2=A0 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.
=C2=A0

> +=C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct vhost_dev *owner =3D virtio_lookup= _vhost_device(&uuid);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (owner =3D=3D NULL || dev !=3D owner) = {

I dev is always set dev !=3D owner should also cover the NULL case.

True, I can remove the NULL case from the cond= ition.
=C2=A0
However will we see uuid's that aren't associated with anything?

Theoretically, it shouldn't happen. D= mabufs 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/maliciou= s
requests, so worth the check.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Not allowed to remove no= n-owned entries */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 default:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Not allowed to remove non-owned entrie= s */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
> +=C2=A0 =C2=A0 }
> +
>=C2=A0 =C2=A0 =C2=A0 return virtio_remove_resource(&uuid);
>=C2=A0 }
>=C2=A0
> @@ -1794,7 +1810,8 @@ static gboolean backend_read(QIOChannel *ioc, GI= OCondition condition,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D vhost_user_backend_handle_sh= ared_object_add(dev, &payload.object);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D vhost_user_backend_handle_shared_= object_remove(&payload.object);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D vhost_user_backend_handle_shared_= object_remove(dev,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&a= mp;payload.object);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D vhost_user_backend_handle_sh= ared_object_lookup(dev->opaque, ioc,

--
Alex Benn=C3=A9e
Virtualisation Tech Lead @ Linaro

--000000000000f084990611685fb8--