Patchwork virtio: abort on zero config length

login
register
mail settings
Submitter Jason Wang
Date April 25, 2013, 7:43 a.m.
Message ID <1366875807-3491-1-git-send-email-jasowang@redhat.com>
Download mbox | patch
Permalink /patch/239412/
State New
Headers show

Comments

Jason Wang - April 25, 2013, 7:43 a.m.
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(-)
Michael S. Tsirkin - April 25, 2013, 8:11 a.m.
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
Anthony Liguori - April 25, 2013, 8:20 p.m.
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
Michael S. Tsirkin - April 25, 2013, 9:02 p.m.
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
Anthony Liguori - April 25, 2013, 10:27 p.m.
"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
Jason Wang - April 26, 2013, 5:06 a.m.
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
Eric Blake - April 26, 2013, 10:32 a.m.
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.
Jason Wang - April 26, 2013, 10:33 a.m.
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.
Laszlo Ersek - April 26, 2013, 11:33 a.m.
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
Michael S. Tsirkin - April 26, 2013, 12:31 p.m.
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.
Anthony Liguori - April 26, 2013, 2:13 p.m.
"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

Patch

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);
 }