Re: [Bug 1888606] [NEW] Heap-use-after-free in virtio_gpu_ctrl_response

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2020年8月3日周一 下午2:57写道：
>
>   Hi,
>
> > > The ASAN trace:
> > > ==29798==ERROR: AddressSanitizer: heap-use-after-free on address 
> > > 0x60d0000050e8 at pc 0x560629814761 bp 0x7ffe916eb1e0 sp 0x7ffe916eb1d8
> > > READ of size 8 at 0x60d0000050e8 thread T0
> > >     #0 0x560629814760 in virtio_gpu_ctrl_response 
> > > /home/alxndr/Development/qemu/hw/display/virtio-gpu.c:181:42
> > >     #4 0x56062a8f1c96 in aio_bh_poll 
> > > /home/alxndr/Development/qemu/util/async.c:164:13
>
> > >     #1 0x560629827730 in virtio_gpu_reset 
> > > /home/alxndr/Development/qemu/hw/display/virtio-gpu.c:1160:9
>
> So it looks like the bottom half accesses stuff released by reset.
>
> Guess the reset should cancel any scheduled bh calls to avoid that ...
>

Gerd, the root case is we do DMA to MMIO. Following happens

1. bh -> virtio_gpu_handle_ctrl
here we alloc 'cmd'

2. Next, virtio_gpu_handle_ctrl...->virtqueue_push->...write virtio
used ring(but here we write to the virtio-gpu's MMIO
and then we go virtio_gpu_reset).
In the virtio_gpu_reset, we free the 'cmd'

3. In the 'virtio_gpu_ctrl_response', we use the 'cmd' again after
'virtqueue_push'
But the 'cmd' has been freed in step two.

So the root cause is the DMA do write to MMIO.
This is a general issue we have discussed in the e1000e case.

We still don't think out a solution for this.

Thanks,
Li Qiang

> Does the patch below help?
>
> thanks,
>   Gerd
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5f0dd7c15002..18f0011b5a0a 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1144,6 +1144,9 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
>      struct virtio_gpu_simple_resource *res, *tmp;
>      struct virtio_gpu_ctrl_command *cmd;
>
> +    qemu_bh_cancel(g->ctrl_bh);
> +    qemu_bh_cancel(g->cursor_bh);
> +
>  #ifdef CONFIG_VIRGL
>      if (g->parent_obj.use_virgl_renderer) {
>          virtio_gpu_virgl_reset(g);
>
>