Re: [PATCH] vhost: Check for valid vdev in vhost_backend_handle_iotlb_msg

From: Eugenio Perez Martin <eperezma@redhat.com>
To: Stefano Garzarella <sgarzare@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-level <qemu-devel@nongnu.org>,
	qemu-stable@nongnu.org
Subject: Re: [PATCH] vhost: Check for valid vdev in vhost_backend_handle_iotlb_msg
Date: Mon, 1 Feb 2021 14:06:53 +0100	[thread overview]
Message-ID: <CAJaqyWferxXcOXftXz8Oz0VunZ7oSh6zc1QdrM8VOnNbFqN1Pw@mail.gmail.com> (raw)
In-Reply-To: <20210201115954.6v6ga7ledlumeby4@steredhat>

On Mon, Feb 1, 2021 at 1:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Jan 29, 2021 at 10:07:28AM +0100, Eugenio Pérez wrote:
> >Not checking this can lead to invalid dev->vdev member access in
> >vhost_device_iotlb_miss if backend issue an iotlb message in a bad
> >timing, either maliciously or by a bug.
> >
> >Reproduced rebooting a guest with testpmd in txonly forward mode.
> > #0  0x0000559ffff94394 in vhost_device_iotlb_miss (
> >     dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
> >     at ../hw/virtio/vhost.c:1013
> > #1  0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
> >     imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
> >     at ../hw/virtio/vhost-backend.c:411
> > #2  vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
> >     imsg=imsg@entry=0x7ffddcfd32c0)
> >     at ../hw/virtio/vhost-backend.c:404
> > #3  0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
> >     at ../hw/virtio/vhost-user.c:1464
> > #4  0x000055a0000c541b in aio_dispatch_handler (
> >     ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
> >     at ../util/aio-posix.c:329
> >
> >Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
>
> I'm not sure but IIUC vhost_backend_handle_iotlb_msg() was introduced by
> commit 020e571b8b, so maybe is better this 'Fixes' line:
>
> Fixes: 020e571b8b ("vhost: rework IOTLB messaging")
>

Hi Stefano.

Thanks for reviewing it :). Actually yes, you are right, I carried the
previous Fixes line by mistake.

Should I send a new patch?

Thanks!

> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> > hw/virtio/vhost-backend.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> >diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> >index 222bbcc62d..31b33bde37 100644
> >--- a/hw/virtio/vhost-backend.c
> >+++ b/hw/virtio/vhost-backend.c
> >@@ -406,6 +406,11 @@ int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> > {
> >     int ret = 0;
> >
> >+    if (unlikely(!dev->vdev)) {
> >+        error_report("Unexpected IOTLB message when virtio device is stopped");
> >+        return -EINVAL;
> >+    }
> >+
> >     switch (imsg->type) {
> >     case VHOST_IOTLB_MISS:
> >         ret = vhost_device_iotlb_miss(dev, imsg->iova,
> >--
> >2.27.0
> >
> >
>
> The patch LGTM:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>