| Message ID | 1b79118e.25c5.17f2702b9d5.Coremail.wliang@stu.xidian.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0). | expand |
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
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
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 >
> > 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
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
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 >
--- ./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; } }
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