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

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

[Jason Wang]

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 <mst@redhat.com>
Cc: Petr Matousek <pmatouse@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 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;
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_readb(proxy->vdev, addr);
@@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
         return;
     }
     addr -= config;
+
+    if (addr + size > proxy->vdev->config_len) {
+        return;
+    }
+
     /*
      * Virtio-PCI is odd. Ioports are LE but config space is target native
      * endian.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1c2282c..3397b5e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
 
     vdev->get_config(vdev, vdev->config);
 
-    if (addr > (vdev->config_len - sizeof(val)))
-        return (uint32_t)-1;
-
     val = ldub_p(vdev->config + addr);
     return val;
 }
@@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr)
 
     vdev->get_config(vdev, vdev->config);
 
-    if (addr > (vdev->config_len - sizeof(val)))
-        return (uint32_t)-1;
-
     val = lduw_p(vdev->config + addr);
     return val;
 }
@@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr)
 
     vdev->get_config(vdev, vdev->config);
 
-    if (addr > (vdev->config_len - sizeof(val)))
-        return (uint32_t)-1;
-
     val = ldl_p(vdev->config + addr);
     return val;
 }
@@ -600,9 +591,6 @@ 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)
@@ -613,9 +601,6 @@ 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)
@@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data)
 {
     uint32_t val = data;
 
-    if (addr > (vdev->config_len - sizeof(val)))
-        return;
-
     stl_p(vdev->config + addr, val);
 
     if (vdev->set_config)
-- 
1.7.1

[Qemu-devel] [PATCH 2/3] virtio-ccw: check config length before accessing it

[Jason Wang]

virtio-rng-ccw has zero config length, so we need validate the config length
before trying to access it. Otherwise we may crash since vdev->config is NULL.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/s390x/virtio-ccw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 56539d3..8d0dff5 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -260,7 +260,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, dev->vdev->config_len);
-        if (!ccw.cda) {
+        if (!ccw.cda || !len) {
             ret = -EFAULT;
         } else {
             dev->vdev->get_config(dev->vdev, dev->vdev->config);
@@ -279,7 +279,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         }
         len = MIN(ccw.count, dev->vdev->config_len);
         hw_len = len;
-        if (!ccw.cda) {
+        if (!ccw.cda || !len) {
             ret = -EFAULT;
         } else {
             config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] virtio-ccw: check config length before accessing it

[Michael S. Tsirkin]

On Fri, Apr 26, 2013 at 04:34:03PM +0800, Jason Wang wrote:
> virtio-rng-ccw has zero config length, so we need validate the config length
> before trying to access it. Otherwise we may crash since vdev->config is NULL.
> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

The real problem is dev->vdev->get_config being NULL,
isn't it? So why not validate it and be done with it?

> ---
>  hw/s390x/virtio-ccw.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 56539d3..8d0dff5 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -260,7 +260,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              }
>          }
>          len = MIN(ccw.count, dev->vdev->config_len);
> -        if (!ccw.cda) {
> +        if (!ccw.cda || !len) {
>              ret = -EFAULT;
>          } else {
>              dev->vdev->get_config(dev->vdev, dev->vdev->config);
> @@ -279,7 +279,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          }
>          len = MIN(ccw.count, dev->vdev->config_len);
>          hw_len = len;
> -        if (!ccw.cda) {
> +        if (!ccw.cda || !len) {
>              ret = -EFAULT;
>          } else {
>              config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 2/3] virtio-ccw: check config length before accessing it

[Jason Wang]

On 04/28/2013 04:32 PM, Michael S. Tsirkin wrote:
> On Fri, Apr 26, 2013 at 04:34:03PM +0800, Jason Wang wrote:
>> virtio-rng-ccw has zero config length, so we need validate the config length
>> before trying to access it. Otherwise we may crash since vdev->config is NULL.
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> The real problem is dev->vdev->get_config being NULL,
> isn't it? So why not validate it and be done with it?

Ok, this looks more clear. Will do it in V2.
>> ---
>>  hw/s390x/virtio-ccw.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index 56539d3..8d0dff5 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -260,7 +260,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>              }
>>          }
>>          len = MIN(ccw.count, dev->vdev->config_len);
>> -        if (!ccw.cda) {
>> +        if (!ccw.cda || !len) {
>>              ret = -EFAULT;
>>          } else {
>>              dev->vdev->get_config(dev->vdev, dev->vdev->config);
>> @@ -279,7 +279,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>          }
>>          len = MIN(ccw.count, dev->vdev->config_len);
>>          hw_len = len;
>> -        if (!ccw.cda) {
>> +        if (!ccw.cda || !len) {
>>              ret = -EFAULT;
>>          } else {
>>              config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
>> -- 
>> 1.7.1

Re: [Qemu-devel] [PATCH 2/3] virtio-ccw: check config length before accessing it

[Jason Wang]

On 04/28/2013 04:40 PM, Jason Wang wrote:
> On 04/28/2013 04:32 PM, Michael S. Tsirkin wrote:
>> On Fri, Apr 26, 2013 at 04:34:03PM +0800, Jason Wang wrote:
>>> virtio-rng-ccw has zero config length, so we need validate the config length
>>> before trying to access it. Otherwise we may crash since vdev->config is NULL.
>>>
>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> The real problem is dev->vdev->get_config being NULL,
>> isn't it? So why not validate it and be done with it?
> Ok, this looks more clear. Will do it in V2.

Recheck the code, looks like {get|set}_config() has been validated in
virtio_bus_get_vdev_config(). So the codes were ok here, will drop this
patch also.
 
>>> ---
>>>  hw/s390x/virtio-ccw.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>> index 56539d3..8d0dff5 100644
>>> --- a/hw/s390x/virtio-ccw.c
>>> +++ b/hw/s390x/virtio-ccw.c
>>> @@ -260,7 +260,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>              }
>>>          }
>>>          len = MIN(ccw.count, dev->vdev->config_len);
>>> -        if (!ccw.cda) {
>>> +        if (!ccw.cda || !len) {
>>>              ret = -EFAULT;
>>>          } else {
>>>              dev->vdev->get_config(dev->vdev, dev->vdev->config);
>>> @@ -279,7 +279,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>          }
>>>          len = MIN(ccw.count, dev->vdev->config_len);
>>>          hw_len = len;
>>> -        if (!ccw.cda) {
>>> +        if (!ccw.cda || !len) {
>>>              ret = -EFAULT;
>>>          } else {
>>>              config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
>>> -- 
>>> 1.7.1
>

[Qemu-devel] [PATCH 3/3] s390-virtio-bus: sync config only when config_len is not zero

[Jason Wang]

virtio-rng-s390 has zero config length, so no need to sync its config otherwise
qemu will crash since vdev->config is NULL.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/s390x/s390-virtio-bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index dabbc2e..0f83516 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -350,6 +350,10 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
     dev->feat_offs = cur_offs + dev->feat_len;
     cur_offs += dev->feat_len * 2;
 
+    if (!dev->vdev->config_len) {
+        return;
+    }
+
     /* Sync config space */
     if (dev->vdev->get_config) {
         dev->vdev->get_config(dev->vdev, dev->vdev->config);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/3] s390-virtio-bus: sync config only when config_len is not zero

[Alexander Graf]

On 26.04.2013, at 10:34, Jason Wang wrote:

> virtio-rng-s390 has zero config length, so no need to sync its config otherwise
> qemu will crash since vdev->config is NULL.

Why is it NULL?


Alex

> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> hw/s390x/s390-virtio-bus.c |    4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index dabbc2e..0f83516 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -350,6 +350,10 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>     dev->feat_offs = cur_offs + dev->feat_len;
>     cur_offs += dev->feat_len * 2;
> 
> +    if (!dev->vdev->config_len) {
> +        return;
> +    }
> +
>     /* Sync config space */
>     if (dev->vdev->get_config) {
>         dev->vdev->get_config(dev->vdev, dev->vdev->config);
> -- 
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH 3/3] s390-virtio-bus: sync config only when config_len is not zero

[Jason Wang]

On 04/27/2013 01:07 AM, Alexander Graf wrote:
> On 26.04.2013, at 10:34, Jason Wang wrote:
>
>> virtio-rng-s390 has zero config length, so no need to sync its config otherwise
>> qemu will crash since vdev->config is NULL.
> Why is it NULL?

As far as I see, virtio-rng's config_len is zero, so in its vdev->config
were set to NULL in virtio_init().
>
>
> Alex
>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> hw/s390x/s390-virtio-bus.c |    4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index dabbc2e..0f83516 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -350,6 +350,10 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>>     dev->feat_offs = cur_offs + dev->feat_len;
>>     cur_offs += dev->feat_len * 2;
>>
>> +    if (!dev->vdev->config_len) {
>> +        return;
>> +    }
>> +
>>     /* Sync config space */
>>     if (dev->vdev->get_config) {
>>         dev->vdev->get_config(dev->vdev, dev->vdev->config);
>> -- 
>> 1.7.1
>>
>>

Re: [Qemu-devel] [PATCH 3/3] s390-virtio-bus: sync config only when config_len is not zero

[Michael S. Tsirkin]

On Fri, Apr 26, 2013 at 04:34:04PM +0800, Jason Wang wrote:
> virtio-rng-s390 has zero config length, so no need to sync its config otherwise
> qemu will crash since vdev->config is NULL.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Actully, it validates get_config so what's the problem here?

> ---
>  hw/s390x/s390-virtio-bus.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index dabbc2e..0f83516 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -350,6 +350,10 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>      dev->feat_offs = cur_offs + dev->feat_len;
>      cur_offs += dev->feat_len * 2;
>  
> +    if (!dev->vdev->config_len) {
> +        return;
> +    }
> +
>      /* Sync config space */
>      if (dev->vdev->get_config) {
>          dev->vdev->get_config(dev->vdev, dev->vdev->config);
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 3/3] s390-virtio-bus: sync config only when config_len is not zero

[Jason Wang]

On 04/28/2013 04:31 PM, Michael S. Tsirkin wrote:
> On Fri, Apr 26, 2013 at 04:34:04PM +0800, Jason Wang wrote:
>> virtio-rng-s390 has zero config length, so no need to sync its config otherwise
>> qemu will crash since vdev->config is NULL.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Actully, it validates get_config so what's the problem here?

Yes, but the it will also pass vdev->config(NULL) to
cpu_physical_memory_write(),  but since the length is zero, we manage to
survive here. I will drop this patch then.
>> ---
>>  hw/s390x/s390-virtio-bus.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index dabbc2e..0f83516 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -350,6 +350,10 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>>      dev->feat_offs = cur_offs + dev->feat_len;
>>      cur_offs += dev->feat_len * 2;
>>  
>> +    if (!dev->vdev->config_len) {
>> +        return;
>> +    }
>> +
>>      /* Sync config space */
>>      if (dev->vdev->get_config) {
>>          dev->vdev->get_config(dev->vdev, dev->vdev->config);
>> -- 
>> 1.7.1

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 <mst@redhat.com>
> Cc: Petr Matousek <pmatouse@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  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

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 <mst@redhat.com>
>> Cc: Petr Matousek <pmatouse@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  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.

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

[Petr Matousek]

On Sat, Apr 27, 2013 at 01:13:16PM +0800, Jason Wang wrote:
> 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 <mst@redhat.com>
> >> Cc: Petr Matousek <pmatouse@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>  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().

Ok, in that case it would make sense to document the preconditions that
assures that addr + size won't overflow. Or add the values in a safe way
(check that the sum is not less than one of the addend).

> > 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.

So the only practical attack (virtio-rng device that has config length
0) can only end in the first page of qemu address space which is on any
not-so-much recent kernel protected by mmap_min_addr and will result in
qemu process crash. Access to pci config space is privileged operation,
so root user in the guest can crash the guest (something that root can
do anyways).

Don't get me wrong, we still need the fix to avoid any potential issues
in the future, but I'm leaning towards not treating this issue as a
security (CVE) one due to the lack of practical exploitability.

@Kurt -- do we assign CVE identifiers to issues that rely on an option
(or lack of) that when set in a way that would allow the issue in
question to be exploited is known to be insecure?

References:
https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg05013.html
https://bugzilla.redhat.com/show_bug.cgi?id=957155

The option is mmap_min_addr which assures that no mapping can be present
at the beginning of the address space and all accesses will result in
sigsegv. Default setting of mmap_min_addr is enough to avoid this issue
from having security consequences. Disabling mmap_min_addr (setting to
0) means that some if not all of the "kernel NULL pointer dereferences"
out there could be used for privilege escalation.

Thanks,
-- 
Petr Matousek / Red Hat Security Response Team

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

[Kurt Seifried]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/28/2013 05:29 AM, Petr Matousek wrote:
> On Sat, Apr 27, 2013 at 01:13:16PM +0800, Jason Wang wrote:
>> 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 <mst@redhat.com> Cc: Petr Matousek
>>>> <pmatouse@redhat.com> Signed-off-by: Jason Wang
>>>> <jasowang@redhat.com> --- 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().
> 
> Ok, in that case it would make sense to document the preconditions
> that assures that addr + size won't overflow. Or add the values in
> a safe way (check that the sum is not less than one of the
> addend).
> 
>>> 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.
> 
> So the only practical attack (virtio-rng device that has config
> length 0) can only end in the first page of qemu address space
> which is on any not-so-much recent kernel protected by
> mmap_min_addr and will result in qemu process crash. Access to pci
> config space is privileged operation, so root user in the guest can
> crash the guest (something that root can do anyways).
> 
> Don't get me wrong, we still need the fix to avoid any potential
> issues in the future, but I'm leaning towards not treating this
> issue as a security (CVE) one due to the lack of practical
> exploitability.
> 
> @Kurt -- do we assign CVE identifiers to issues that rely on an
> option (or lack of) that when set in a way that would allow the
> issue in question to be exploited is known to be insecure?
> 
> References: 
> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg05013.html
>
> 
https://bugzilla.redhat.com/show_bug.cgi?id=957155
> 
> The option is mmap_min_addr which assures that no mapping can be
> present at the beginning of the address space and all accesses will
> result in sigsegv. Default setting of mmap_min_addr is enough to
> avoid this issue from having security consequences. Disabling
> mmap_min_addr (setting to 0) means that some if not all of the
> "kernel NULL pointer dereferences" out there could be used for
> privilege escalation.
> 
> Thanks,
> 

Ok so this does NOT affect Linux systems using mmap_min_addr with a
sane default value (e.g. larger than 0). However more than a few
systems have shipped with mmap_min_addr set to 0, not picking on
Debian, just the first result I found:

http://wiki.debian.org/mmap_min_addr

"In Debian 5.0.0 through 5.0.3 inclusive, the 2.6.26 kernel is shipped
with a default mmap_min_addr of '0'."

Hopefully everyone has upgraded to 5.0.4 =).

So there is a realistic potential for systems to be affected (e.g. if
this had been fixed 10 years ago I'd have probably not given a CVE,
but 3 years is not a super long time).

Please use CVE-2013-2016 for virtio-pci: properly validate address
before accessing config.



- -- 
Kurt Seifried Red Hat Security Response Team (SRT)
PGP: 0x5E267993 A90B F995 7350 148F 66BF 7554 160D 4553 5E26 7993
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)

iQIcBAEBAgAGBQJRfiKUAAoJEBYNRVNeJnmTcQ0P/iCx/Wz1piIDwvtcEHDrrgJK
pSEztM+1RUC8fsqPjwyaOijupGhMMtmtfJpuhlNnzNUSaWpX7p75d7JMMOWsTztI
wkxOPIso0Zrj7susqYrg7Qiw6G9FJ6sTZFcSWbWFFHieMiVZZMZ0zKe8Vqh1gnv6
cy8OPIDPRT7mkj8lRhYvvzZjIA4S4cNs3DCSvqYn/vwVX/XQI7IQ9f5zz6wrCjLm
prZjHKzZj1MO6C0HLdTw7TWd/gauhbPRyxn13Kb7sk6CeoAx1ip2AX6pS5xILLPE
RV55j46MRosqr9C+V9I2ljQTkRUATLbIM8ny7mxHwB1JYtSn6djL3ID4GJz5Q3RY
TiCiLACqlahxOWv3UDtx60+mOeiZvhE8gP8TS4LexeqIYIuppP93xq1NHZgqYcQT
q/YXxTXBQ/oueXXd2ktiiHwCyWj8FIqnsEIUIDSifpXmlbLPtVxD4MDmqHm+P34m
e4Po3kD/y5AO04qy7M1TbyHunIOXBzQnAmNlzAD4Iw/ou0PSuZP8hB+ZCtJUObKv
BxHXsLGieHmgZ/7qkkxLw9XAZtW6A9Fu+0yHoU7Ur60YQTg/urZhAuFSv6Cm8vti
YRq/xL2+GWG5SzKOBPJ28TUy1bpb3m0tg4bhzzqo8ikruKXr/RhvsQ2BPDaA47F0
oN6qXXGSlpx3XlpE29sr
=IDuh
-----END PGP SIGNATURE-----

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

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 01:34:44AM -0600, Kurt Seifried wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/28/2013 05:29 AM, Petr Matousek wrote:
> > On Sat, Apr 27, 2013 at 01:13:16PM +0800, Jason Wang wrote:
> >> 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 <mst@redhat.com> Cc: Petr Matousek
> >>>> <pmatouse@redhat.com> Signed-off-by: Jason Wang
> >>>> <jasowang@redhat.com> --- 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().
> > 
> > Ok, in that case it would make sense to document the preconditions
> > that assures that addr + size won't overflow. Or add the values in
> > a safe way (check that the sum is not less than one of the
> > addend).
> > 
> >>> 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.
> > 
> > So the only practical attack (virtio-rng device that has config
> > length 0) can only end in the first page of qemu address space
> > which is on any not-so-much recent kernel protected by
> > mmap_min_addr and will result in qemu process crash. Access to pci
> > config space is privileged operation, so root user in the guest can
> > crash the guest (something that root can do anyways).
> > 
> > Don't get me wrong, we still need the fix to avoid any potential
> > issues in the future, but I'm leaning towards not treating this
> > issue as a security (CVE) one due to the lack of practical
> > exploitability.
> > 
> > @Kurt -- do we assign CVE identifiers to issues that rely on an
> > option (or lack of) that when set in a way that would allow the
> > issue in question to be exploited is known to be insecure?
> > 
> > References: 
> > https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg05013.html
> >
> > 
> https://bugzilla.redhat.com/show_bug.cgi?id=957155
> > 
> > The option is mmap_min_addr which assures that no mapping can be
> > present at the beginning of the address space and all accesses will
> > result in sigsegv. Default setting of mmap_min_addr is enough to
> > avoid this issue from having security consequences. Disabling
> > mmap_min_addr (setting to 0) means that some if not all of the
> > "kernel NULL pointer dereferences" out there could be used for
> > privilege escalation.
> > 
> > Thanks,
> > 
> 
> Ok so this does NOT affect Linux systems using mmap_min_addr with a
> sane default value (e.g. larger than 0). However more than a few
> systems have shipped with mmap_min_addr set to 0, not picking on
> Debian, just the first result I found:
> 
> http://wiki.debian.org/mmap_min_addr
> 
> "In Debian 5.0.0 through 5.0.3 inclusive, the 2.6.26 kernel is shipped
> with a default mmap_min_addr of '0'."
> 
> Hopefully everyone has upgraded to 5.0.4 =).
> 
> So there is a realistic potential for systems to be affected (e.g. if
> this had been fixed 10 years ago I'd have probably not given a CVE,
> but 3 years is not a super long time).
> 
> Please use CVE-2013-2016 for virtio-pci: properly validate address
> before accessing config.
> 

Please note this only affects virtio-rng which appeared in qemu v1.3.0 and on,
released on Mon Dec 3 2012. So you'd need a distro with an old kernel
that has an updated qemu or qemu-kvm (or backported the bug in virtio-rng).

-- 
MST

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

[Michael S. Tsirkin]

On Fri, Apr 26, 2013 at 04:27:55PM +0200, 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 <mst@redhat.com>
> > Cc: Petr Matousek <pmatouse@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  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?

Good point.
I think it's only the first page of the qemu address space.




> -- 
> Petr Matousek / Red Hat Security Response Team

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

[Michael S. Tsirkin]

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 <mst@redhat.com>
> Cc: Petr Matousek <pmatouse@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Why do this in virtio-pci and not in virtio.c?
If instead we correct the checks in virtio.c we
get less code, and all transports will benefit
automatically.

> ---
>  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;
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_readb(proxy->vdev, addr);
> @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
>          return;
>      }
>      addr -= config;
> +
> +    if (addr + size > proxy->vdev->config_len) {
> +        return;
> +    }
> +
>      /*
>       * Virtio-PCI is odd. Ioports are LE but config space is target native
>       * endian.
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1c2282c..3397b5e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
>  
>      vdev->get_config(vdev, vdev->config);
>  
> -    if (addr > (vdev->config_len - sizeof(val)))
> -        return (uint32_t)-1;
> -
>      val = ldub_p(vdev->config + addr);
>      return val;
>  }
> @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr)
>  
>      vdev->get_config(vdev, vdev->config);
>  
> -    if (addr > (vdev->config_len - sizeof(val)))
> -        return (uint32_t)-1;
> -
>      val = lduw_p(vdev->config + addr);
>      return val;
>  }
> @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr)
>  
>      vdev->get_config(vdev, vdev->config);
>  
> -    if (addr > (vdev->config_len - sizeof(val)))
> -        return (uint32_t)-1;
> -
>      val = ldl_p(vdev->config + addr);
>      return val;
>  }
> @@ -600,9 +591,6 @@ 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)
> @@ -613,9 +601,6 @@ 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)
> @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data)
>  {
>      uint32_t val = data;
>  
> -    if (addr > (vdev->config_len - sizeof(val)))
> -        return;
> -
>      stl_p(vdev->config + addr, val);
>  
>      if (vdev->set_config)
> -- 
> 1.7.1

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

[Jason Wang]

On 04/28/2013 03:26 AM, Michael S. Tsirkin 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 <mst@redhat.com>
>> Cc: Petr Matousek <pmatouse@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Why do this in virtio-pci and not in virtio.c?
> If instead we correct the checks in virtio.c we
> get less code, and all transports will benefit
> automatically.

I wish I could but looks like vitio_config_read{b|w|l} were only used by
virtio-pci. Other transport such as ccw and s390-virtio-bus have their
own implementation.
>
>> ---
>>  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;
>> +    }
>> +
>>      switch (size) {
>>      case 1:
>>          val = virtio_config_readb(proxy->vdev, addr);
>> @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
>>          return;
>>      }
>>      addr -= config;
>> +
>> +    if (addr + size > proxy->vdev->config_len) {
>> +        return;
>> +    }
>> +
>>      /*
>>       * Virtio-PCI is odd. Ioports are LE but config space is target native
>>       * endian.
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 1c2282c..3397b5e 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
>>  
>>      vdev->get_config(vdev, vdev->config);
>>  
>> -    if (addr > (vdev->config_len - sizeof(val)))
>> -        return (uint32_t)-1;
>> -
>>      val = ldub_p(vdev->config + addr);
>>      return val;
>>  }
>> @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr)
>>  
>>      vdev->get_config(vdev, vdev->config);
>>  
>> -    if (addr > (vdev->config_len - sizeof(val)))
>> -        return (uint32_t)-1;
>> -
>>      val = lduw_p(vdev->config + addr);
>>      return val;
>>  }
>> @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr)
>>  
>>      vdev->get_config(vdev, vdev->config);
>>  
>> -    if (addr > (vdev->config_len - sizeof(val)))
>> -        return (uint32_t)-1;
>> -
>>      val = ldl_p(vdev->config + addr);
>>      return val;
>>  }
>> @@ -600,9 +591,6 @@ 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)
>> @@ -613,9 +601,6 @@ 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)
>> @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data)
>>  {
>>      uint32_t val = data;
>>  
>> -    if (addr > (vdev->config_len - sizeof(val)))
>> -        return;
>> -
>>      stl_p(vdev->config + addr, val);
>>  
>>      if (vdev->set_config)
>> -- 
>> 1.7.1

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

[Michael S. Tsirkin]

On Sun, Apr 28, 2013 at 03:54:20PM +0800, Jason Wang wrote:
> On 04/28/2013 03:26 AM, Michael S. Tsirkin 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 <mst@redhat.com>
> >> Cc: Petr Matousek <pmatouse@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Why do this in virtio-pci and not in virtio.c?
> > If instead we correct the checks in virtio.c we
> > get less code, and all transports will benefit
> > automatically.
> 
> I wish I could but looks like vitio_config_read{b|w|l} were only used by
> virtio-pci. Other transport such as ccw and s390-virtio-bus have their
> own implementation.

Okay but still, the bug is in checks in virtio.c, why not fix it there
instead of making it assume caller does the checks?

> >
> >> ---
> >>  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;
> >> +    }
> >> +
> >>      switch (size) {
> >>      case 1:
> >>          val = virtio_config_readb(proxy->vdev, addr);
> >> @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
> >>          return;
> >>      }
> >>      addr -= config;
> >> +
> >> +    if (addr + size > proxy->vdev->config_len) {
> >> +        return;
> >> +    }
> >> +
> >>      /*
> >>       * Virtio-PCI is odd. Ioports are LE but config space is target native
> >>       * endian.
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 1c2282c..3397b5e 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
> >>  
> >>      vdev->get_config(vdev, vdev->config);
> >>  
> >> -    if (addr > (vdev->config_len - sizeof(val)))
> >> -        return (uint32_t)-1;
> >> -
> >>      val = ldub_p(vdev->config + addr);
> >>      return val;
> >>  }
> >> @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr)
> >>  
> >>      vdev->get_config(vdev, vdev->config);
> >>  
> >> -    if (addr > (vdev->config_len - sizeof(val)))
> >> -        return (uint32_t)-1;
> >> -
> >>      val = lduw_p(vdev->config + addr);
> >>      return val;
> >>  }
> >> @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr)
> >>  
> >>      vdev->get_config(vdev, vdev->config);
> >>  
> >> -    if (addr > (vdev->config_len - sizeof(val)))
> >> -        return (uint32_t)-1;
> >> -
> >>      val = ldl_p(vdev->config + addr);
> >>      return val;
> >>  }
> >> @@ -600,9 +591,6 @@ 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)
> >> @@ -613,9 +601,6 @@ 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)
> >> @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data)
> >>  {
> >>      uint32_t val = data;
> >>  
> >> -    if (addr > (vdev->config_len - sizeof(val)))
> >> -        return;
> >> -
> >>      stl_p(vdev->config + addr, val);
> >>  
> >>      if (vdev->set_config)
> >> -- 
> >> 1.7.1

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

[Andreas Färber]

Am 28.04.2013 10:35, schrieb Michael S. Tsirkin:
> On Sun, Apr 28, 2013 at 03:54:20PM +0800, Jason Wang wrote:
>> On 04/28/2013 03:26 AM, Michael S. Tsirkin 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 <mst@redhat.com>
>>>> Cc: Petr Matousek <pmatouse@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Why do this in virtio-pci and not in virtio.c?
>>> If instead we correct the checks in virtio.c we
>>> get less code, and all transports will benefit
>>> automatically.
>>
>> I wish I could but looks like vitio_config_read{b|w|l} were only used by
>> virtio-pci. Other transport such as ccw and s390-virtio-bus have their
>> own implementation.
> 
> Okay but still, the bug is in checks in virtio.c, why not fix it there
> instead of making it assume caller does the checks?

Ping? This issue has been assigned a CVE but the solution does not seem
to be agreed on yet - are you working on a different proposal, Jason?

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Jason Wang]

On 05/02/2013 10:35 PM, Andreas Färber wrote:
> Am 28.04.2013 10:35, schrieb Michael S. Tsirkin:
>> On Sun, Apr 28, 2013 at 03:54:20PM +0800, Jason Wang wrote:
>>> On 04/28/2013 03:26 AM, Michael S. Tsirkin 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 <mst@redhat.com>
>>>>> Cc: Petr Matousek <pmatouse@redhat.com>
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> Why do this in virtio-pci and not in virtio.c?
>>>> If instead we correct the checks in virtio.c we
>>>> get less code, and all transports will benefit
>>>> automatically.
>>> I wish I could but looks like vitio_config_read{b|w|l} were only used by
>>> virtio-pci. Other transport such as ccw and s390-virtio-bus have their
>>> own implementation.
>> Okay but still, the bug is in checks in virtio.c, why not fix it there
>> instead of making it assume caller does the checks?
> Ping? This issue has been assigned a CVE but the solution does not seem
> to be agreed on yet - are you working on a different proposal, Jason?
>
> Thanks,
> Andreas
>

Hi, I was just back from vacation, will draft V2 soon according to
Michael's comments.

Thanks