Re: [Qemu-devel] [PATCH] virtio: abort on zero config length

[Jason Wang <jasowang@redhat.com>]

On 04/26/2013 06:27 AM, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> On Thu, Apr 25, 2013 at 03:20:20PM -0500, Anthony Liguori wrote:
>>> Jason Wang <jasowang@redhat.com> writes:
>>>
>>>> In fact we don't support zero length config length for virtio device.
>>> virtio-rng?
>> It has config_len == 0?  In that case guest using virtio-rng can crash
>> qemu or read qemu memory:
>>
>> uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
>> {
>>     uint8_t val;
>>
>>     vdev->get_config(vdev, vdev->config);
>>
>>     if (addr > (vdev->config_len - sizeof(val)))
>>
>> ^^^^^^^^^ quiz: spot a bug above if config_len is 0    :)
> Then we need to fix these bugs and allocate a CVE.  virtio-rng has
> shipped.  This code is also dumb.

Ok, but since the discussion is in public list, no need for CVE then.
>
> There's no requirement that config_len is >= 4 so this would fail
> equally awfully with config_len=1|2|3.

Will check if (addr + size > vdev->config_len) in the caller for both
read and write.

Thanks
>
> Regards,
>
> Anthony Liguori
>
>>
>>         return (uint32_t)-1;
>>
>>     val = ldub_p(vdev->config + addr);
>>     return val;
>> }
>>
>> how about corrupting qemu memory? That too:
>>
>> void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data)
>> {
>>     uint8_t val = data;
>>
>>     if (addr > (vdev->config_len - sizeof(val)))
>>         return;
>>
>>     stb_p(vdev->config + addr, val);
>>
>>     if (vdev->set_config)
>>         vdev->set_config(vdev, vdev->config);
>> }
>>
>>
>>
>>>> And it can lead outbound memory access. So abort on zero config length
>>>> to catch the bug earlier.
>>> Not sure what you mean, but virtio-rng has a zero length config space.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>  hw/virtio/virtio.c |    7 ++-----
>>>>  1 files changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>> index 1c2282c..a6fa667 100644
>>>> --- a/hw/virtio/virtio.c
>>>> +++ b/hw/virtio/virtio.c
>>>> @@ -923,6 +923,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>>>>                   uint16_t device_id, size_t config_size)
>>>>  {
>>>>      int i;
>>>> +    assert(config_size);
>>>>      vdev->device_id = device_id;
>>>>      vdev->status = 0;
>>>>      vdev->isr = 0;
>>>> @@ -938,11 +939,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>>>>  
>>>>      vdev->name = name;
>>>>      vdev->config_len = config_size;
>>>> -    if (vdev->config_len) {
>>>> -        vdev->config = g_malloc0(config_size);
>>>> -    } else {
>>>> -        vdev->config = NULL;
>>>> -    }
>>>> +    vdev->config = g_malloc0(config_size);
>>>>      vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
>>>>                                                       vdev);
>>>>  }
>>>> -- 
>>>> 1.7.1