From: Wentao Liang <Wentao_Liang_g@163.com>

A potential Use-after-free was reported in virtio_iommu_handle_command
when using virtio-iommu:

> I find a potential Use-after-free in QEMU 6.2.0, which is in
> virtio_iommu_handle_command() (./hw/virtio/virtio-iommu.c).
>
>
> Specifically, in the loop body, the variable 'buf' allocated at line 639 can be
> freed by g_free() at line 659. However, if the execution path enters the loop
> body again and the if branch takes true at line 616, the control will directly
> jump to 'out' at line 651. At this time, 'buf' is a freed pointer, which is not
> assigned with an allocated memory but used at line 653. As a result, a UAF bug
> is triggered.
>
>
>
> 599     for (;;) {
> ...
> 615         sz = iov_to_buf(iov, iov_cnt, 0, &head, sizeof(head));
> 616         if (unlikely(sz != sizeof(head))) {
> 617             tail.status = VIRTIO_IOMMU_S_DEVERR;
> 618             goto out;
> 619         }
> ...
> 639             buf = g_malloc0(output_size);
> ...
> 651 out:
> 652         sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> 653                           buf ? buf : &tail, output_size);
> ...
> 659         g_free(buf);
>
> We can fix it by set ‘buf‘ to NULL after freeing it:
>
>
> 651 out:
> 652         sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> 653                           buf ? buf : &tail, output_size);
> ...
> 659         g_free(buf);
> +++ buf = NULL;
> 660     }

Fix as suggested by the reporter.

Signed-off-by: Wentao Liang <Wentao_Liang_g@163.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Message-ID: <20220406040445-mutt-send-email-mst@kernel.org>
---
 hw/virtio/virtio-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 239fe97b12..2b1d21edd1 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -683,6 +683,7 @@ out:
         virtio_notify(vdev, vq);
         g_free(elem);
         g_free(buf);
+        buf = NULL;
     }
 }
 
-- 
MST

On Thu, 7 Apr 2022 at 10:52, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Wentao Liang <Wentao_Liang_g@163.com>
>
> A potential Use-after-free was reported in virtio_iommu_handle_command
> when using virtio-iommu:
>
> > I find a potential Use-after-free in QEMU 6.2.0, which is in
> > virtio_iommu_handle_command() (./hw/virtio/virtio-iommu.c).

So, this isn't a regression. Do you think it's critically necessary
it goes in 7.0, or is it in the category "put it into 7.0 if we
need an rc4 for some other reason anyway" ?

(I have a feeling we'll need an rc4, but we'll see.)

thanks
-- PMM

On Thu, Apr 07, 2022 at 11:03:16AM +0100, Peter Maydell wrote:
> On Thu, 7 Apr 2022 at 10:52, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Wentao Liang <Wentao_Liang_g@163.com>
> >
> > A potential Use-after-free was reported in virtio_iommu_handle_command
> > when using virtio-iommu:
> >
> > > I find a potential Use-after-free in QEMU 6.2.0, which is in
> > > virtio_iommu_handle_command() (./hw/virtio/virtio-iommu.c).
> 
> So, this isn't a regression. Do you think it's critically necessary
> it goes in 7.0, or is it in the category "put it into 7.0 if we
> need an rc4 for some other reason anyway" ?
> 
> (I have a feeling we'll need an rc4, but we'll see.)
> 
> thanks
> -- PMM

I am concerned it can be used to trigger a CVE but I could not
find a way. So I would say if there's an rc4 pls include it
but if not then we can pick it up in stable.

-- 
MST

On Thu, 7 Apr 2022 at 15:50, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 07, 2022 at 11:03:16AM +0100, Peter Maydell wrote:
> > On Thu, 7 Apr 2022 at 10:52, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > From: Wentao Liang <Wentao_Liang_g@163.com>
> > >
> > > A potential Use-after-free was reported in virtio_iommu_handle_command
> > > when using virtio-iommu:
> > >
> > > > I find a potential Use-after-free in QEMU 6.2.0, which is in
> > > > virtio_iommu_handle_command() (./hw/virtio/virtio-iommu.c).
> >
> > So, this isn't a regression. Do you think it's critically necessary
> > it goes in 7.0, or is it in the category "put it into 7.0 if we
> > need an rc4 for some other reason anyway" ?
> >
> > (I have a feeling we'll need an rc4, but we'll see.)
> >
> > thanks
> > -- PMM
>
> I am concerned it can be used to trigger a CVE but I could not
> find a way. So I would say if there's an rc4 pls include it
> but if not then we can pick it up in stable.

We needed an rc4 for a couple of other security fixes, so I've
applied this to master; thanks.

-- PMM