| Message ID | 1366875807-3491-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
"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
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
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.
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.
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
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.
"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
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); }
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(-)