Message ID | 1475659998-22045-1-git-send-email-kraxel@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 05.10.2016 11:33, Gerd Hoffmann wrote: > snprintf return value is *not* the number of chars written into the > buffer, but the number of chars needed. So in case the buffer is too > small you can go alloc a bigger one and try again. But that also means > you can't simply use the return value for the next snprintf call > without checking beforehand that things did actually fit. > > Problem is that usb_desc_create_serial didn't perform that check, so a > loooong path string (can happen with deep pci-bridge nesting) results in > the third snprintf call smashing the stack. > > Fix this by throwing out all the snpintf calls and use g_strdup_printf > instead. > > https://bugzilla.redhat.com/show_bug.cgi?id=1381630 > > Reported-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/usb/desc.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/usb/desc.c b/hw/usb/desc.c > index 5e0e1d1..7828e52 100644 > --- a/hw/usb/desc.c > +++ b/hw/usb/desc.c > @@ -556,9 +556,7 @@ void usb_desc_create_serial(USBDevice *dev) > DeviceState *hcd = dev->qdev.parent_bus->parent; > const USBDesc *desc = usb_device_get_usb_desc(dev); > int index = desc->id.iSerialNumber; > - char serial[64]; > - char *path; > - int dst; > + char *path, *serial; > > if (dev->serial) { > /* 'serial' usb bus property has priority if present */ > @@ -567,14 +565,16 @@ void usb_desc_create_serial(USBDevice *dev) > } > > assert(index != 0 && desc->str[index] != NULL); > - dst = snprintf(serial, sizeof(serial), "%s", desc->str[index]); > path = qdev_get_dev_path(hcd); > if (path) { > - dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", path); > + serial = g_strdup_printf("%s-%s-%s", desc->str[index], > + path, dev->port->path); > + } else { > + serial = g_strdup_printf("%s-%s", desc->str[index], dev->port->path); > } > - dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", dev->port->path); > usb_desc_set_string(dev, index, serial); > g_free(path); > + g_free(serial); > } > > const char *usb_desc_get_string(USBDevice *dev, uint8_t index) Reviewed-by: Thomas Huth <thuth@redhat.com>
On 10/05/2016 04:33 AM, Gerd Hoffmann wrote: > snprintf return value is *not* the number of chars written into the > buffer, but the number of chars needed. So in case the buffer is too > small you can go alloc a bigger one and try again. But that also means > you can't simply use the return value for the next snprintf call > without checking beforehand that things did actually fit. > > Problem is that usb_desc_create_serial didn't perform that check, so a > loooong path string (can happen with deep pci-bridge nesting) results in > the third snprintf call smashing the stack. Is this exploitable enough to need a CVE? > > Fix this by throwing out all the snpintf calls and use g_strdup_printf s/snpintf/snprintf/ > instead. > > https://bugzilla.redhat.com/show_bug.cgi?id=1381630 > > Reported-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/usb/desc.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Hi, > > Problem is that usb_desc_create_serial didn't perform that check, so a > > loooong path string (can happen with deep pci-bridge nesting) results in > > the third snprintf call smashing the stack. > > Is this exploitable enough to need a CVE? It isn't guest-triggerable. Also it needs a pretty unusual config to happen (pci-bridges nested so deep that lspci -t inside the guest crashes). So I'd rate it pretty low on the severity scale. > > Fix this by throwing out all the snpintf calls and use g_strdup_printf > > s/snpintf/snprintf/ Fixed. cheers, Gerd
diff --git a/hw/usb/desc.c b/hw/usb/desc.c index 5e0e1d1..7828e52 100644 --- a/hw/usb/desc.c +++ b/hw/usb/desc.c @@ -556,9 +556,7 @@ void usb_desc_create_serial(USBDevice *dev) DeviceState *hcd = dev->qdev.parent_bus->parent; const USBDesc *desc = usb_device_get_usb_desc(dev); int index = desc->id.iSerialNumber; - char serial[64]; - char *path; - int dst; + char *path, *serial; if (dev->serial) { /* 'serial' usb bus property has priority if present */ @@ -567,14 +565,16 @@ void usb_desc_create_serial(USBDevice *dev) } assert(index != 0 && desc->str[index] != NULL); - dst = snprintf(serial, sizeof(serial), "%s", desc->str[index]); path = qdev_get_dev_path(hcd); if (path) { - dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", path); + serial = g_strdup_printf("%s-%s-%s", desc->str[index], + path, dev->port->path); + } else { + serial = g_strdup_printf("%s-%s", desc->str[index], dev->port->path); } - dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", dev->port->path); usb_desc_set_string(dev, index, serial); g_free(path); + g_free(serial); } const char *usb_desc_get_string(USBDevice *dev, uint8_t index)
snprintf return value is *not* the number of chars written into the buffer, but the number of chars needed. So in case the buffer is too small you can go alloc a bigger one and try again. But that also means you can't simply use the return value for the next snprintf call without checking beforehand that things did actually fit. Problem is that usb_desc_create_serial didn't perform that check, so a loooong path string (can happen with deep pci-bridge nesting) results in the third snprintf call smashing the stack. Fix this by throwing out all the snpintf calls and use g_strdup_printf instead. https://bugzilla.redhat.com/show_bug.cgi?id=1381630 Reported-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/usb/desc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)