| Message ID | 20210505130716.1128420-5-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/6] hw/usb/host-stub: Remove unused header | expand |
On Wed, May 05, 2021 at 03:07:14PM +0200, Gerd Hoffmann wrote: >[...] >diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c >index 17f06f34179a..6a75b0dc4ab2 100644 >--- a/hw/usb/redirect.c >+++ b/hw/usb/redirect.c >@@ -620,7 +620,7 @@ static void usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p, > .endpoint = ep, > .length = p->iov.size > }; >- uint8_t buf[p->iov.size]; >+ g_autofree uint8_t *buf = g_malloc(p->iov.size); > /* No id, we look at the ep when receiving a status back */ > usb_packet_copy(p, buf, p->iov.size); > usbredirparser_send_iso_packet(dev->parser, 0, &iso_packet, >@@ -818,7 +818,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p, > usbredirparser_send_bulk_packet(dev->parser, p->id, > &bulk_packet, NULL, 0); > } else { >- uint8_t buf[size]; >+ g_autofree uint8_t *buf = g_malloc(size); > usb_packet_copy(p, buf, size); Won't this allows us to malloc then write an arbitrary amount of heap memory, allowing a guest driver to abort the qemu ? Remy
On Wed, May 05, 2021 at 03:29:10PM +0200, Remy Noel wrote: > On Wed, May 05, 2021 at 03:07:14PM +0200, Gerd Hoffmann wrote: > > [...] > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > > index 17f06f34179a..6a75b0dc4ab2 100644 > > --- a/hw/usb/redirect.c > > +++ b/hw/usb/redirect.c > > @@ -620,7 +620,7 @@ static void usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p, > > .endpoint = ep, > > .length = p->iov.size > > }; > > - uint8_t buf[p->iov.size]; > > + g_autofree uint8_t *buf = g_malloc(p->iov.size); > > /* No id, we look at the ep when receiving a status back */ > > usb_packet_copy(p, buf, p->iov.size); > > usbredirparser_send_iso_packet(dev->parser, 0, &iso_packet, > > @@ -818,7 +818,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p, > > usbredirparser_send_bulk_packet(dev->parser, p->id, > > &bulk_packet, NULL, 0); > > } else { > > - uint8_t buf[size]; > > + g_autofree uint8_t *buf = g_malloc(size); > > usb_packet_copy(p, buf, size); > > Won't this allows us to malloc then write an arbitrary amount of heap > memory, allowing a guest driver to abort the qemu ? Crashing qemu is not as easy as with stack allocation, but yes, unbound allocation is still there. Need to figure some way to limit this in xhci without breaking things. Or maybe use g_try_malloc() and catch allocation failures. Alternatively we could add a usbredirparser_send_bulk_packet_iov() function to usbredir so we can just pass on the iov and don't need a temporary buffer in the first place. Not sure yet what the best way forward is. Comments (and other ideas) are welcome. take care, Gerd
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index 17f06f34179a..6a75b0dc4ab2 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -620,7 +620,7 @@ static void usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p, .endpoint = ep, .length = p->iov.size }; - uint8_t buf[p->iov.size]; + g_autofree uint8_t *buf = g_malloc(p->iov.size); /* No id, we look at the ep when receiving a status back */ usb_packet_copy(p, buf, p->iov.size); usbredirparser_send_iso_packet(dev->parser, 0, &iso_packet, @@ -818,7 +818,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p, usbredirparser_send_bulk_packet(dev->parser, p->id, &bulk_packet, NULL, 0); } else { - uint8_t buf[size]; + g_autofree uint8_t *buf = g_malloc(size); usb_packet_copy(p, buf, size); usbredir_log_data(dev, "bulk data out:", buf, size); usbredirparser_send_bulk_packet(dev->parser, p->id, @@ -923,7 +923,7 @@ static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev, USBPacket *p, uint8_t ep) { struct usb_redir_interrupt_packet_header interrupt_packet; - uint8_t buf[p->iov.size]; + g_autofree uint8_t *buf = g_malloc(p->iov.size); DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep, p->iov.size, p->id);
Use autofree heap allocation instead. Fixes: 4f4321c11ff ("usb: use iovecs in USBPacket") Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Message-Id: <20210503132915.2335822-3-kraxel@redhat.com> --- hw/usb/redirect.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)