* [Qemu-devel] [PATCH] virtio: abort on zero config length @ 2013-04-25 7:43 Jason Wang 2013-04-25 8:11 ` Michael S. Tsirkin 2013-04-25 20:20 ` Anthony Liguori 0 siblings, 2 replies; 11+ messages in thread From: Jason Wang @ 2013-04-25 7:43 UTC (permalink / raw) To: aliguori, qemu-devel, mst; +Cc: 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-25 7:43 [Qemu-devel] [PATCH] virtio: abort on zero config length Jason Wang @ 2013-04-25 8:11 ` Michael S. Tsirkin 2013-04-25 20:20 ` Anthony Liguori 1 sibling, 0 replies; 11+ messages in thread From: Michael S. Tsirkin @ 2013-04-25 8:11 UTC (permalink / raw) To: Jason Wang; +Cc: aliguori, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-25 7:43 [Qemu-devel] [PATCH] virtio: abort on zero config length Jason Wang 2013-04-25 8:11 ` Michael S. Tsirkin @ 2013-04-25 20:20 ` Anthony Liguori 2013-04-25 21:02 ` Michael S. Tsirkin 1 sibling, 1 reply; 11+ messages in thread From: Anthony Liguori @ 2013-04-25 20:20 UTC (permalink / raw) To: Jason Wang, qemu-devel, mst 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-25 20:20 ` Anthony Liguori @ 2013-04-25 21:02 ` Michael S. Tsirkin 2013-04-25 22:27 ` Anthony Liguori 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2013-04-25 21:02 UTC (permalink / raw) To: Anthony Liguori; +Cc: Jason Wang, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-25 21:02 ` Michael S. Tsirkin @ 2013-04-25 22:27 ` Anthony Liguori 2013-04-26 5:06 ` Jason Wang 0 siblings, 1 reply; 11+ messages in thread From: Anthony Liguori @ 2013-04-25 22:27 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Jason Wang, qemu-devel "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-25 22:27 ` Anthony Liguori @ 2013-04-26 5:06 ` Jason Wang 2013-04-26 10:32 ` Eric Blake 0 siblings, 1 reply; 11+ messages in thread From: Jason Wang @ 2013-04-26 5:06 UTC (permalink / raw) To: Anthony Liguori; +Cc: qemu-devel, Michael S. Tsirkin 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-26 5:06 ` Jason Wang @ 2013-04-26 10:32 ` Eric Blake 2013-04-26 10:33 ` Jason Wang 2013-04-26 11:33 ` Laszlo Ersek 0 siblings, 2 replies; 11+ messages in thread From: Eric Blake @ 2013-04-26 10:32 UTC (permalink / raw) To: Jason Wang; +Cc: Anthony Liguori, qemu-devel, Michael S. Tsirkin [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-26 10:32 ` Eric Blake @ 2013-04-26 10:33 ` Jason Wang 2013-04-26 12:31 ` Michael S. Tsirkin 2013-04-26 11:33 ` Laszlo Ersek 1 sibling, 1 reply; 11+ messages in thread From: Jason Wang @ 2013-04-26 10:33 UTC (permalink / raw) To: Eric Blake; +Cc: Anthony Liguori, qemu-devel, Michael S. Tsirkin 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-26 10:33 ` Jason Wang @ 2013-04-26 12:31 ` Michael S. Tsirkin 2013-04-26 14:13 ` Anthony Liguori 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2013-04-26 12:31 UTC (permalink / raw) To: Jason Wang; +Cc: Anthony Liguori, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-26 12:31 ` Michael S. Tsirkin @ 2013-04-26 14:13 ` Anthony Liguori 0 siblings, 0 replies; 11+ messages in thread From: Anthony Liguori @ 2013-04-26 14:13 UTC (permalink / raw) To: Michael S. Tsirkin, Jason Wang; +Cc: qemu-devel "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on zero config length 2013-04-26 10:32 ` Eric Blake 2013-04-26 10:33 ` Jason Wang @ 2013-04-26 11:33 ` Laszlo Ersek 1 sibling, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2013-04-26 11:33 UTC (permalink / raw) To: Eric Blake; +Cc: Jason Wang, Michael S. Tsirkin, Anthony Liguori, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-04-26 14:14 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-25 7:43 [Qemu-devel] [PATCH] virtio: abort on zero config length Jason Wang 2013-04-25 8:11 ` Michael S. Tsirkin 2013-04-25 20:20 ` Anthony Liguori 2013-04-25 21:02 ` Michael S. Tsirkin 2013-04-25 22:27 ` Anthony Liguori 2013-04-26 5:06 ` Jason Wang 2013-04-26 10:32 ` Eric Blake 2013-04-26 10:33 ` Jason Wang 2013-04-26 12:31 ` Michael S. Tsirkin 2013-04-26 14:13 ` Anthony Liguori 2013-04-26 11:33 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).