Re: [Qemu-devel] [PATCH 1/3] virtio-pci: properly validate address before accessing config

[Jason Wang]

On 04/26/2013 10:27 PM, Petr Matousek wrote:
> On Fri, Apr 26, 2013 at 04:34:02PM +0800, Jason Wang wrote:
>> There are several several issues in the current checking:
>>
>> - The check was based on the minus of unsigned values which can overflow
>> - It was done after .{set|get}_config() which can lead crash when config_len 
>> is
>>   zero since vdev->config is NULL
>>
>> Fix this by:
>>
>> - Validate the address in virtio_pci_config_{read|write}() before
>>   .{set|get}_config
>> - Use addition instead minus to do the validation
>>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Cc: Petr Matousek <address@hidden>
>> Signed-off-by: Jason Wang <address@hidden>
>> ---
>>  hw/virtio/virtio-pci.c |    9 +++++++++
>>  hw/virtio/virtio.c     |   18 ------------------
>>  2 files changed, 9 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index a1f15a8..7f6c7d1 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -400,6 +400,10 @@ static uint64_t virtio_pci_config_read(void *opaque, 
>> hwaddr addr,
>>      }
>>      addr -= config;
>>  
>> +    if (addr + size > proxy->vdev->config_len) {
>> +        return (uint32_t)-1;
>> +    }
>> +
> What is the range of values addr can be? I guess it's not arbitrary and
> not fully in guests hands. Can it be higher than corresponding pci
> config space size?

Not fully in guests hands. It depends on size the config size.
Unfortunately, qemu will roundup the size to power of 2 in
virtio_pci_device_plugged():

    size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
         + virtio_bus_get_vdev_config_len(bus);

    if (size & (size - 1)) {
        size = 1 << qemu_fls(size);
    }

So, for virtio-rng, though its region size is 20, it will be rounded up
to 32, which left guest the possibility to access beyond the config
space. So some check is needs in virito_pci_config_read().
>
> IOW, can guest touch anything interesting or will all accesses end in
> the first page in the qemu address space, considering vdev->config being
> NULL?
>

There's another theoretical issue as pointed by Anthony, see
virtio_config_writew():

void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data)
{
    uint16_t val = data;

    if (addr > (vdev->config_len - sizeof(val)))
        return;

    stw_p(vdev->config + addr, val);

    if (vdev->set_config)
        vdev->set_config(vdev, vdev->config);
}

If there's a device whose config_len is 1, the check will fail and we
can access some other location.

But since virtio-rng has zero config length and addr here should be less
than 12, and all other device's config length is all greater than 4.
Only first page could be access here.