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
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
© 2016 - 2025 Red Hat, Inc.