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

[Petr Matousek]

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?

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?

-- 
Petr Matousek / Red Hat Security Response Team