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

From: Eric Auger <eric.auger@redhat.com>
To: Thomas Huth <thuth@redhat.com>,
	wliang@stu.xidian.edu.cn, "Michael S. Tsirkin" <mst@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: Fix a potential Use-after-free in virtio_iommu_handle_command() (v6.2.0).
Date: Wed, 23 Feb 2022 18:39:02 +0100	[thread overview]
Message-ID: <a300d2f3-e545-7793-2601-fec1cbbd601a@redhat.com> (raw)
In-Reply-To: <3ce08bdb-fecd-549a-5c09-0b5fa65de4ba@redhat.com>

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
>