diff mbox series

xhci: check return value from usb_packet_map

Message ID 20200827115933.1851563-1-ppandit@redhat.com
State New
Headers show
Series xhci: check return value from usb_packet_map | expand

Commit Message

P J P Aug. 27, 2020, 11:59 a.m. UTC
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(-)

Comments

Alexander Bulekov Aug. 30, 2020, 3:27 p.m. UTC | #1
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
> 
>
P J P Sept. 1, 2020, 4:49 a.m. UTC | #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
Gerd Hoffmann Sept. 1, 2020, 5:27 a.m. UTC | #3
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 mbox series

Patch

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;