Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[wliang]

[-- Attachment #1.1: Type: text/plain, Size: 1298 bytes --]

Hi all,

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);
...
651out:
652        sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
653                          buf ? buf : &tail, output_size);
...
659        g_free(buf);
660    }






We can fix it by set ‘buf‘ to NULL after freeing it:



651out:
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    }


I'm looking forward to your confirmation.


Best,
Wentao

[-- Attachment #1.2: Type: text/html, Size: 7522 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: virtio-iommu.c.patch --]
[-- Type: text/x-patch; name=virtio-iommu.c.patch, Size: 272 bytes --]

--- ./hw/virtio/virtio-iommu.c	2022-02-23 15:06:32.040727196 +0800
+++ ./hw/virtio/virtio-iommu-PATCH.c	2022-02-23 21:12:24.605032121 +0800
@@ -657,6 +657,7 @@
         virtio_notify(vdev, vq);
         g_free(elem);
         g_free(buf);
+        buf = NULL;
     }
 }
 

Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[Thomas Huth]

On 23/02/2022 15.36, wliang@stu.xidian.edu.cn wrote:
> Hi all,
> 
> 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);
> ...
> 651out:
> 652        sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> 653                          buf ? buf : &tail, output_size);
> ...
> 659        g_free(buf);
> 660    }
> 
> 
> We can fix it by set ‘buf‘ to NULL after freeing it:
> 
> 
> 
> 651out:
> 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    }
> 
> 
> I'm looking forward to your confirmation.

  Hi,

thanks for your report and patch - but to make sure that the right people 
get attention, please use the scripts/get_maintainer.pl script to get a list 
of people who should be on CC:, or look into the MAINTAINERS file directly 
(for the next time - this time, I've CC:ed them now already).

  Thanks,
   Thomas

Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[Philippe Mathieu-Daudé]

On 23/2/22 17:02, Thomas Huth wrote:
> On 23/02/2022 15.36, wliang@stu.xidian.edu.cn wrote:
>> Hi all,
>>
>> I find a potential Use-after-free in QEMU 6.2.0, which is in 
>> virtio_iommu_handle_command() (./hw/virtio/virtio-iommu.c).

>> I'm looking forward to your confirmation.
> 
>   Hi,
> 
> thanks for your report and patch - but to make sure that the right 
> people get attention, please use the scripts/get_maintainer.pl script to 
> get a list of people who should be on CC:, or look into the MAINTAINERS 
> file directly (for the next time - this time, I've CC:ed them now already).

You can find the contribution guidelines here:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html

Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[Eric Auger]

Hi,

On 2/23/22 5:02 PM, Thomas Huth wrote:
> On 23/02/2022 15.36, wliang@stu.xidian.edu.cn wrote:
>> Hi all,
>>
>> 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);
>> ...
>> 651out:
>> 652        sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
>> 653                          buf ? buf : &tail, output_size);
>> ...
>> 659        g_free(buf);
>> 660    }
>>
>>
>> We can fix it by set ‘buf‘ to NULL after freeing it:
>>
>>
>>
>> 651out:
>> 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    }
>>
>>
>> I'm looking forward to your confirmation.
Thank you for the report. Yes setting buff to null after the g_free
looks the right thing to do here. Please feel free to send the patch.
>
>  Hi,
>
> thanks for your report and patch - but to make sure that the right
> people get attention, please use the scripts/get_maintainer.pl script
> to get a list of people who should be on CC:, or look into the
> MAINTAINERS file directly (for the next time - this time, I've CC:ed
> them now already).
Thanks you Thomas for the cc ;-)

Eric
>
>  Thanks,
>   Thomas
>

Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[wliang]

> > thanks for your report and patch - but to make sure that the right 
> > people get attention, please use the scripts/get_maintainer.pl script to 
> > get a list of people who should be on CC:, or look into the MAINTAINERS 
> > file directly (for the next time - this time, I've CC:ed them now already).
> 
> You can find the contribution guidelines here:
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html



Thank you so much!
You guys are so kid! That reminds me how beautiful the world is.
Have a good day!

Thanks,
Wentao

Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[wliang]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]

Hi all,

Here is a new patch with Signed-off-by tags.
The old one is wrong for it did't have Signed-off-by tags.
I am looking forward to your confirmation.

Thanks,
Wentao

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-a-potential-Use-after-free-in-virtio_iommu_handl.patch --]
[-- Type: text/x-patch;  name=0001-Fix-a-potential-Use-after-free-in-virtio_iommu_handl.patch, Size: 680 bytes --]

From 8ece42bda1099a9a0df584cac2478ec5a6e83924 Mon Sep 17 00:00:00 2001
From: Wentao_Liang <Wentao_Liang_g@163.com>
Date: Fri, 25 Feb 2022 11:49:54 +0800
Subject: [PATCH] Fix a potential Use-after-free in
 virtio_iommu_handle_command() (v6.2.0).

Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
---
 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 aa9c16a17b..a394901347 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -657,6 +657,7 @@ out:
         virtio_notify(vdev, vq);
         g_free(elem);
         g_free(buf);
+        buf = NULL;
     }
 }
 
-- 
2.25.1

Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).

[Michael S. Tsirkin]

On Fri, Feb 25, 2022 at 11:58:43AM +0800, wliang@stu.xidian.edu.cn wrote:
> Hi all,
> 
> Here is a new patch with Signed-off-by tags.
> The old one is wrong for it did't have Signed-off-by tags.
> I am looking forward to your confirmation.
> 
> Thanks,
> Wentao

> From 8ece42bda1099a9a0df584cac2478ec5a6e83924 Mon Sep 17 00:00:00 2001
> From: Wentao_Liang <Wentao_Liang_g@163.com>
> Date: Fri, 25 Feb 2022 11:49:54 +0800
> Subject: [PATCH] Fix a potential Use-after-free in
>  virtio_iommu_handle_command() (v6.2.0).
> 
> Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
> ---
>  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 aa9c16a17b..a394901347 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -657,6 +657,7 @@ out:
>          virtio_notify(vdev, vq);
>          g_free(elem);
>          g_free(buf);
> +        buf = NULL;
>      }
>  }

I merged this fix, adding the commit log description.
I also note it should be sent inline not as an attachment.

Thanks a lot for the contribution!

> -- 
> 2.25.1
>