Re: Null-pointer dereference through virtio-balloon

[David Hildenbrand]

On 16.05.20 13:35, Philippe Mathieu-Daudé wrote:
> +David (virtio-balloon maintainer)
> 
> On Mon, May 11, 2020 at 6:42 AM Alexander Bulekov <address@hidden> wrote:
>>
>> Hello,
>> While fuzzing, I found an input that triggers a null-ptr dereference in
>> aio_bh_enqueue, through virtio-balloon. Based on the stacktrace below,
>> I am not positive that this is specific to virtio-balloon, however
>> I have not encountered the same issue for any of the other virtio
>> devices I am fuzzing.
>>
>> AddressSanitizer: SEGV on unknown address 0x000000000000
>>
>> #0 0x55ee5b93eb28 in aio_bh_enqueue util/async.c:69:27
>> #1 0x55ee5b93eb28 in qemu_bh_schedule util/async.c:181:5
>> #2 0x55ee5ae71465 in virtio_queue_notify hw/virtio/virtio.c:2364:9
>> #3 0x55ee5b51142d in virtio_mmio_write hw/virtio/virtio-mmio.c:369:13
>> #4 0x55ee5ad0d2d6 in memory_region_write_accessor memory.c:483:5
>> #5 0x55ee5ad0cc7f in access_with_adjusted_size memory.c:544:18
>> #6 0x55ee5ad0cc7f in memory_region_dispatch_write memory.c:1476:16
>> #7 0x55ee5ac221d3 in flatview_write_continue exec.c:3137:23
>> #8 0x55ee5ac1ab97 in flatview_write exec.c:3177:14
>> #9 0x55ee5ac1ab97 in address_space_write exec.c:3268:18
>>
>> I can reproduce it in a qemu 5.0 build using:
>> cat << EOF | qemu-system-i386 -M pc-q35-5.0 -M 
>> microvm,x-option-roms=off,pit=off,pic=off,isa-serial=off,rtc=off -nographic 
>> -device virtio-balloon-device,free-page-hint=true,deflate-on-oom=true 
>> -nographic -monitor none -display none -serial none -qtest stdio
>> write 0xc0000e30 0x24 
>> 0x030000000300000003000000030000000300000003000000030000000300000003000000
>> EOF
> 
> If you start QEMU this way, you get a warning:
> 
> qemu-system-i386: -device
> virtio-balloon-device,free-page-hint=true,deflate-on-oom=true:
> iothread is missing
> 
>         if (s->iothread) {
>             s->free_page_bh =
> aio_bh_new(iothread_get_aio_context(s->iothread), ...
>             ...
>         } else {
>             ...
>            virtio_error(vdev, "iothread is missing");
>         }
> 
> Shouldn't we call error_setg(errp, "iothread is missing") and return instead?

Yes, that error handling is just messed up. The issue is that we create
a queue but leave free_page_bh set to 0. The virtio_error() will be
cleared by a device reset, the feature is not indicated but the queue
exists and can be notified.

Somebody requested the feature, so we should bail out if we cannot make
it work.

I'll send a fix, thanks!

-- 
Thanks,

David / dhildenb