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

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

[Jason Wang]

In fact we don't support zero length config length for virtio device. And it can
lead outbound memory access. So abort on zero config length to catch the bug
earlier.

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

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

[Michael S. Tsirkin]

On Thu, Apr 25, 2013 at 03:43:27PM +0800, Jason Wang wrote:
> In fact we don't support zero length config length for virtio device. And it can
> lead outbound memory access. So abort on zero config length to catch the bug
> earlier.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@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

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

[Anthony Liguori]

Jason Wang <jasowang@redhat.com> writes:

> In fact we don't support zero length config length for virtio device.

virtio-rng?

> 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

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

[Michael S. Tsirkin]

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    :)


        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

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

[Anthony Liguori]

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

There's no requirement that config_len is >= 4 so this would fail
equally awfully with config_len=1|2|3.

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

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

[Jason Wang]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

On 04/25/2013 11:06 PM, Jason Wang wrote:
>>>     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.

Wrong.  CVEs are useful even for publicly disclosed bugs.  It tells
people whether they need to upgrade in order to avoid a vulnerability.

What we don't need is embargo.  But we do need a CVE.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Jason Wang]

On 04/26/2013 06:32 PM, Eric Blake wrote:
> On 04/25/2013 11:06 PM, Jason Wang wrote:
>>>>     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.
> Wrong.  CVEs are useful even for publicly disclosed bugs.  It tells
> people whether they need to upgrade in order to avoid a vulnerability.
>
> What we don't need is embargo.  But we do need a CVE.
>

True, thanks for the correction.

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

[Laszlo Ersek]

On 04/26/13 12:32, Eric Blake wrote:
> On 04/25/2013 11:06 PM, Jason Wang wrote:
>>>>     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.
> 
> Wrong.  CVEs are useful even for publicly disclosed bugs.  It tells
> people whether they need to upgrade in order to avoid a vulnerability.

Small addition (since my English parser turns "whether" into "whether or
not"): a CVE tells people *that* (not "if") they should upgrade. Lack of
a CVE mention in a commit doesn't imply -- at least in, ugh, another big
project -- that the fix is without security consequences ("no CVE fix, I
don't need it").

(Apologies for hair-splitting.)

Laszlo

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

[Michael S. Tsirkin]

On Fri, Apr 26, 2013 at 06:33:33PM +0800, Jason Wang wrote:
> On 04/26/2013 06:32 PM, Eric Blake wrote:
> > On 04/25/2013 11:06 PM, Jason Wang wrote:
> >>>>     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.
> > Wrong.  CVEs are useful even for publicly disclosed bugs.  It tells
> > people whether they need to upgrade in order to avoid a vulnerability.
> >
> > What we don't need is embargo.  But we do need a CVE.
> >
> 
> True, thanks for the correction.

I think we never shipped QEMU release with this bug.  So no need for
CVEs.  I'm not sure upstream has to bother with CVEs - we can just say
this is downstream work.

-- 
MST

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Fri, Apr 26, 2013 at 06:33:33PM +0800, Jason Wang wrote:
>> On 04/26/2013 06:32 PM, Eric Blake wrote:
>> > On 04/25/2013 11:06 PM, Jason Wang wrote:
>> >>>>     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.
>> > Wrong.  CVEs are useful even for publicly disclosed bugs.  It tells
>> > people whether they need to upgrade in order to avoid a vulnerability.
>> >
>> > What we don't need is embargo.  But we do need a CVE.
>> >
>> 
>> True, thanks for the correction.
>
> I think we never shipped QEMU release with this bug.  So no need for
> CVEs.  I'm not sure upstream has to bother with CVEs - we can just say
> this is downstream work.

Uh, we certainly do.  QEMU 1.4 had virtio-rng and therefore had this bug
so we need to allocate a CVE.

Regards,

Anthony Liguori

>
> -- 
> MST