| Message ID | 20200827115933.1851563-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | xhci: check return value from usb_packet_map | expand |
I think there is already a fix queued for this one: https://www.mail-archive.com/qemu-devel@nongnu.org/msg734424.html On 200827 1729, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While setting up a packet in xhci_setup_packet() routine, > usb_packet_map() may return an error. Check this return value > before further processing the packet, to avoid use-after-free > issue. > > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fxhci_uaf_2 > #0 __interceptor_free (/lib64/libasan.so.6+0xb0307) > #1 qemu_vfree ../util/oslib-posix.c:247 > #2 address_space_unmap ../exec.c:3635 > #3 dma_memory_unmap ../include/sysemu/dma.h:145 > #4 usb_packet_unmap ../hw/usb/libhw.c:65 > #5 usb_packet_map ../hw/usb/libhw.c:54 > #6 xhci_setup_packet ../hw/usb/hcd-xhci.c:1618 > #7 xhci_fire_ctl_transfer ../hw/usb/hcd-xhci.c:1722 > #8 xhci_kick_epctx ../hw/usb/hcd-xhci.c:1991 > #9 xhci_kick_ep ../hw/usb/hcd-xhci.c:1861 > #10 xhci_doorbell_write ../hw/usb/hcd-xhci.c:3162 > ... > > Reported-by: Ruhr-University <bugs-syssec@rub.de> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/usb/hcd-xhci.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 67a18fe2b6..848e7e935f 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -1615,7 +1615,14 @@ static int xhci_setup_packet(XHCITransfer *xfer) > xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */ > usb_packet_setup(&xfer->packet, dir, ep, xfer->streamid, > xfer->trbs[0].addr, false, xfer->int_req); > - usb_packet_map(&xfer->packet, &xfer->sgl); > + if (usb_packet_map(&xfer->packet, &xfer->sgl) < 0) { > + DPRINTF("xhci: setup packet failed: pid: 0x%x addr %d ep %d\n", > + xfer->packet.pid, ep->dev->addr, ep->nr); > + usb_packet_cleanup(&xfer->packet); > + qemu_sglist_destroy(&xfer->sgl); > + return -1; > + } > + > DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n", > xfer->packet.pid, ep->dev->addr, ep->nr); > return 0; > -- > 2.26.2 > >
+-- On Sun, 30 Aug 2020, Alexander Bulekov wrote --+
| I think there is already a fix queued for this one:
| https://www.mail-archive.com/qemu-devel@nongnu.org/msg734424.html
Yes, it looks similar.
| > @@ -1615,7 +1615,14 @@ static int xhci_setup_packet(XHCITransfer *xfer)
| > xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */
| > usb_packet_setup(&xfer->packet, dir, ep, xfer->streamid,
| > xfer->trbs[0].addr, false, xfer->int_req);
| > - usb_packet_map(&xfer->packet, &xfer->sgl);
| > + if (usb_packet_map(&xfer->packet, &xfer->sgl) < 0) {
| > + DPRINTF("xhci: setup packet failed: pid: 0x%x addr %d ep %d\n",
| > + xfer->packet.pid, ep->dev->addr, ep->nr);
| > + usb_packet_cleanup(&xfer->packet);
| > + qemu_sglist_destroy(&xfer->sgl);
| > + return -1;
We don't need 'usb_packet_cleanup' call? (to confirm)
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On Tue, Sep 01, 2020 at 10:19:26AM +0530, P J P wrote: > +-- On Sun, 30 Aug 2020, Alexander Bulekov wrote --+ > | I think there is already a fix queued for this one: > | https://www.mail-archive.com/qemu-devel@nongnu.org/msg734424.html > > Yes, it looks similar. > > | > @@ -1615,7 +1615,14 @@ static int xhci_setup_packet(XHCITransfer *xfer) > | > xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */ > | > usb_packet_setup(&xfer->packet, dir, ep, xfer->streamid, > | > xfer->trbs[0].addr, false, xfer->int_req); > | > - usb_packet_map(&xfer->packet, &xfer->sgl); > | > + if (usb_packet_map(&xfer->packet, &xfer->sgl) < 0) { > | > + DPRINTF("xhci: setup packet failed: pid: 0x%x addr %d ep %d\n", > | > + xfer->packet.pid, ep->dev->addr, ep->nr); > | > + usb_packet_cleanup(&xfer->packet); > | > + qemu_sglist_destroy(&xfer->sgl); > | > + return -1; > > We don't need 'usb_packet_cleanup' call? (to confirm) Oh, didn't notice the difference. I think we need it, otherwise we leak iov entries in case the packet has multiple segments and only the second (or any later) fails to map. take care, Gerd
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 67a18fe2b6..848e7e935f 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -1615,7 +1615,14 @@ static int xhci_setup_packet(XHCITransfer *xfer) xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */ usb_packet_setup(&xfer->packet, dir, ep, xfer->streamid, xfer->trbs[0].addr, false, xfer->int_req); - usb_packet_map(&xfer->packet, &xfer->sgl); + if (usb_packet_map(&xfer->packet, &xfer->sgl) < 0) { + DPRINTF("xhci: setup packet failed: pid: 0x%x addr %d ep %d\n", + xfer->packet.pid, ep->dev->addr, ep->nr); + usb_packet_cleanup(&xfer->packet); + qemu_sglist_destroy(&xfer->sgl); + return -1; + } + DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n", xfer->packet.pid, ep->dev->addr, ep->nr); return 0;