[PATCH for-8.1 v2] hw/virtio-iommu: Fix potential OOB access in virtio_iommu_handle_command()

[PATCH for-8.1 v2] hw/virtio-iommu: Fix potential OOB access in virtio_iommu_handle_command()

[Eric Auger]

In the virtio_iommu_handle_command() when a PROBE request is handled,
output_size takes a value greater than the tail size and on a subsequent
iteration we can get a stack out-of-band access. Initialize the
output_size on each iteration.

The issue was found with ASAN. Credits to:
Yiming Tao(Zhejiang University)
Gaoning Pan(Zhejiang University)

Fixes: 1733eebb9e7 ("virtio-iommu: Implement RESV_MEM probe request")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Mauro Matteo Cascella <mcascell@redhat.com>
Cc: qemu-stable@nongnu.org

---
- added the Cc: qemu-stable@nongnu.org and copied 2 persons involved
  in the reporting loop
---
 hw/virtio/virtio-iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 201127c488..4dcf1d5c62 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -728,13 +728,15 @@ static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
     struct virtio_iommu_req_head head;
     struct virtio_iommu_req_tail tail = {};
-    size_t output_size = sizeof(tail), sz;
     VirtQueueElement *elem;
     unsigned int iov_cnt;
     struct iovec *iov;
     void *buf = NULL;
+    size_t sz;
 
     for (;;) {
+        size_t output_size = sizeof(tail);
+
         elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
         if (!elem) {
             return;
-- 
2.38.1

Re: [PATCH for-8.1 v2] hw/virtio-iommu: Fix potential OOB access in virtio_iommu_handle_command()

[Philippe Mathieu-Daudé]

On 17/7/23 18:21, Eric Auger wrote:
> In the virtio_iommu_handle_command() when a PROBE request is handled,
> output_size takes a value greater than the tail size and on a subsequent
> iteration we can get a stack out-of-band access. Initialize the
> output_size on each iteration.

A backtrace would be helpful (or reproducer).

> The issue was found with ASAN. Credits to:
> Yiming Tao(Zhejiang University)
> Gaoning Pan(Zhejiang University)
> 
> Fixes: 1733eebb9e7 ("virtio-iommu: Implement RESV_MEM probe request")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Mauro Matteo Cascella <mcascell@redhat.com>

Did you mean:

Reported-by: Yiming Tao <taoym@zju.edu.cn>

?

> Cc: qemu-stable@nongnu.org
> 
> ---
> - added the Cc: qemu-stable@nongnu.org and copied 2 persons involved
>    in the reporting loop
> ---
>   hw/virtio/virtio-iommu.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.1 v2] hw/virtio-iommu: Fix potential OOB access in virtio_iommu_handle_command()

[Eric Auger]

Hi Philippe,

On 7/17/23 19:26, Philippe Mathieu-Daudé wrote:
> On 17/7/23 18:21, Eric Auger wrote:
>> In the virtio_iommu_handle_command() when a PROBE request is handled,
>> output_size takes a value greater than the tail size and on a subsequent
>> iteration we can get a stack out-of-band access. Initialize the
>> output_size on each iteration.
>
> A backtrace would be helpful (or reproducer).
Well the way to hit it in explained above. PROBE request followed by a
different request consumed within the same virtio_iommu_handle_command()
call
>
>> The issue was found with ASAN. Credits to:
>> Yiming Tao(Zhejiang University)
>> Gaoning Pan(Zhejiang University)
>>
>> Fixes: 1733eebb9e7 ("virtio-iommu: Implement RESV_MEM probe request")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Mauro Matteo Cascella <mcascell@redhat.com>
>
> Did you mean:
>
> Reported-by: Yiming Tao <taoym@zju.edu.cn>
>
> ?
If I understand correctly trhe actual people who found the issue using
the tool were those I mentionned in the credits section in the commit
msg. but unfortunately I don't have their full address. Mauro Matteo
forwarded me the report which transited though Yiming Tao. Anyway I will
be glad to put more precise in the R-b tags if I get some additional info.

Thanks

Eric
>
>> Cc: qemu-stable@nongnu.org
>>
>> ---
>> - added the Cc: qemu-stable@nongnu.org and copied 2 persons involved
>>    in the reporting loop
>> ---
>>   hw/virtio/virtio-iommu.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>