On 04/26/2013 06:27 AM, Anthony Liguori wrote:
> "Michael S. Tsirkin" <m...@redhat.com> writes:
>
>> On Thu, Apr 25, 2013 at 03:20:20PM -0500, Anthony Liguori wrote:
>>> Jason Wang <jasow...@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 <jasow...@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
