Quantcast

Fix multiple USB use-after-free

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Fix multiple USB use-after-free

Martin Pieuchot
In polling mode, finished transfers are processed by the waiting thread.
This happens inside usbd_dopoll().  That means it's unsafe to dereference
``xfer'' after calling it:

352: usbd_dopoll(pipe->device);
353: if (xfer->done)
354: break;


When interrupts are enabled and a transfer times out it is unsafe to
dereference ``xfer'' after being awaken.  That's because the aborting
task calls wakeup(9) after executing the callback:


807: pipe->methods->done(xfer);
808: if (xfer->callback)
809: xfer->callback(xfer, xfer->priv, xfer->status);

                ...

821: if ((flags & USBD_SYNCHRONOUS) && !polling)
822: wakeup(xfer);


These problems are inherent to the fact that the ``done'' flag is on
the ``xfer'' descriptor.

The diff below fixes the problem by using a unique ID to check if a
submitted transfer is finished or not.  Since we know that the ``pipe''
associated to a transfer wont disappear until it is closed, we can use
it to store the current ID.
When can also tsleep(9) on the address of the pipe instead of the address
of the transfer.

Note that with this change usbd_transfer() will no longer report all
possible USB error messages.  But I'd say it's an improvement since
not a single driver can handle them.

ok?

Index: ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.197
diff -u -p -r1.197 ehci.c
--- ehci.c 10 Mar 2017 11:18:48 -0000 1.197
+++ ehci.c 10 Mar 2017 12:00:57 -0000
@@ -3348,7 +3348,6 @@ ehci_device_isoc_start(struct usbd_xfer
 
  TAILQ_INSERT_TAIL(&sc->sc_intrhead, ex, inext);
  xfer->status = USBD_IN_PROGRESS;
- xfer->done = 0;
  splx(s);
 
  return (USBD_IN_PROGRESS);
Index: usbdi.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.89
diff -u -p -r1.89 usbdi.c
--- usbdi.c 10 Mar 2017 11:18:48 -0000 1.89
+++ usbdi.c 10 Mar 2017 12:00:58 -0000
@@ -54,6 +54,8 @@ extern int usbdebug;
 #define DPRINTFN(n,x)
 #endif
 
+uint64_t usbd_xfer_id; /* unique ID per xfer */
+
 void usbd_request_async_cb(struct usbd_xfer *, void *, usbd_status);
 void usbd_start_next(struct usbd_pipe *pipe);
 usbd_status usbd_open_pipe_ival(struct usbd_interface *, u_int8_t, u_int8_t,
@@ -282,6 +284,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  struct usbd_bus *bus = pipe->device->bus;
  int polling = bus->use_polling;
  usbd_status err;
+ uint32_t timeout;
+ uint64_t uxid;
  int flags, s;
 
  if (usbd_is_dying(pipe->device))
@@ -293,8 +297,6 @@ usbd_transfer(struct usbd_xfer *xfer)
  if (usbdebug > 5)
  usbd_dump_queue(pipe);
 #endif
- xfer->done = 0;
-
  if (pipe->aborting)
  return (USBD_CANCELLED);
 
@@ -320,8 +322,12 @@ usbd_transfer(struct usbd_xfer *xfer)
  usb_syncmem(&xfer->dmabuf, 0, xfer->length,
     BUS_DMASYNC_PREREAD);
 
- err = pipe->methods->transfer(xfer);
+ /* We cannot dereference ``xfer'' after giving it to the HC. */
+ uxid = xfer->id;
+ flags = xfer->flags;
+ timeout = xfer->timeout;
 
+ err = pipe->methods->transfer(xfer);
  if (err != USBD_IN_PROGRESS && err != USBD_NORMAL_COMPLETION) {
  /* The transfer has not been queued, so free buffer. */
  if (xfer->rqflags & URQ_AUTO_DMABUF) {
@@ -330,50 +336,52 @@ usbd_transfer(struct usbd_xfer *xfer)
  }
  }
 
- if (!(xfer->flags & USBD_SYNCHRONOUS))
+ if (!(flags & USBD_SYNCHRONOUS) || (err != USBD_IN_PROGRESS))
  return (err);
 
  /* Sync transfer, wait for completion. */
- if (err != USBD_IN_PROGRESS)
- return (err);
-
  s = splusb();
  if (polling) {
  int timo;
 
- for (timo = xfer->timeout; timo >= 0; timo--) {
+ for (timo = timeout; timo >= 0; timo--) {
  usb_delay_ms(bus, 1);
  if (bus->dying) {
- xfer->status = USBD_IOERROR;
- usb_transfer_complete(xfer);
+ err = USBD_IOERROR;
  break;
  }
 
  usbd_dopoll(pipe->device);
- if (xfer->done)
+ if (pipe->uxid != uxid) {
+ err = USBD_NORMAL_COMPLETION;
  break;
+ }
  }
 
- if (timo < 0) {
- xfer->status = USBD_TIMEOUT;
- usb_transfer_complete(xfer);
- }
+ if (timo < 0)
+ err = USBD_TIMEOUT;
  } else {
- while (!xfer->done) {
- flags = PRIBIO|(xfer->flags & USBD_CATCH ? PCATCH : 0);
+ int error;
 
- err = tsleep(xfer, flags, "usbsyn", 0);
- if (err && !xfer->done) {
- usbd_abort_pipe(pipe);
- if (err == EINTR)
- xfer->status = USBD_INTERRUPTED;
+ while (pipe->uxid == uxid) {
+ flags = PRIBIO | (flags & USBD_CATCH ? PCATCH : 0);
+
+ error = tsleep(pipe, flags, "usbsyn", 0);
+ if (error && (pipe->uxid == uxid)) {
+ if (error == EINTR)
+ err = USBD_INTERRUPTED;
  else
- xfer->status = USBD_TIMEOUT;
+ err = USBD_TIMEOUT;
+ } else {
+ err = USBD_NORMAL_COMPLETION;
  }
  }
  }
+ if (err)
+ usbd_abort_pipe(pipe);
  splx(s);
- return (xfer->status);
+
+ return (err);
 }
 
 void *
@@ -414,6 +422,12 @@ usbd_alloc_xfer(struct usbd_device *dev)
  xfer = dev->bus->methods->allocx(dev->bus);
  if (xfer == NULL)
  return (NULL);
+
+ usbd_xfer_id++;
+ if (usbd_xfer_id == 0)
+ usbd_xfer_id++;
+
+ xfer->id = usbd_xfer_id;
 #ifdef DIAGNOSTIC
  xfer->busy_free = XFER_FREE;
 #endif
@@ -727,19 +741,6 @@ usb_transfer_complete(struct usbd_xfer *
  printf("%s: xfer=%p not on queue\n", __func__, xfer);
  return;
  }
-#endif
-
-#ifdef DIAGNOSTIC
- if (pipe == NULL) {
- printf("usb_transfer_complete: pipe==0, xfer=%p\n", xfer);
- return;
- }
-#endif
- /* XXXX */
- if (polling)
- pipe->running = 0;
-
-#ifdef DIAGNOSTIC
  if (xfer->actlen > xfer->length) {
  printf("%s: actlen > len %u > %u\n", __func__, xfer->actlen,
     xfer->length);
@@ -747,6 +748,9 @@ usb_transfer_complete(struct usbd_xfer *
  }
 #endif
 
+ if (polling)
+ pipe->running = 0;
+
  if (xfer->actlen != 0) {
  if (usbd_xfer_isread(xfer)) {
  usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
@@ -784,7 +788,7 @@ usb_transfer_complete(struct usbd_xfer *
  ++pipe->device->bus->stats.uds_requests
  [pipe->endpoint->edesc->bmAttributes & UE_XFERTYPE];
 
- xfer->done = 1;
+ pipe->uxid = 0;
  if (!xfer->status && xfer->actlen < xfer->length &&
     !(xfer->flags & USBD_SHORT_XFER_OK)) {
  DPRINTFN(-1,("usb_transfer_complete: short transfer %d<%d\n",
@@ -819,7 +823,7 @@ usb_transfer_complete(struct usbd_xfer *
  pipe->repeat = 0;
 
  if ((flags & USBD_SYNCHRONOUS) && !polling)
- wakeup(xfer);
+ wakeup(pipe);
 
  if (!pipe->repeat) {
  /* XXX should we stop the queue on all errors? */
@@ -853,6 +857,7 @@ usb_insert_transfer(struct usbd_xfer *xf
  if (pipe->running)
  err = USBD_IN_PROGRESS;
  else {
+ pipe->uxid = xfer->id;
  pipe->running = 1;
  err = USBD_NORMAL_COMPLETION;
  }
@@ -860,7 +865,6 @@ usb_insert_transfer(struct usbd_xfer *xf
  return (err);
 }
 
-/* Called at splusb() */
 void
 usbd_start_next(struct usbd_pipe *pipe)
 {
@@ -869,23 +873,13 @@ usbd_start_next(struct usbd_pipe *pipe)
 
  SPLUSBCHECK;
 
-#ifdef DIAGNOSTIC
- if (pipe == NULL) {
- printf("usbd_start_next: pipe == NULL\n");
- return;
- }
- if (pipe->methods == NULL || pipe->methods->start == NULL) {
- printf("usbd_start_next: pipe=%p no start method\n", pipe);
- return;
- }
-#endif
-
  /* Get next request in queue. */
  xfer = SIMPLEQ_FIRST(&pipe->queue);
  DPRINTFN(5, ("usbd_start_next: pipe=%p, xfer=%p\n", pipe, xfer));
  if (xfer == NULL) {
  pipe->running = 0;
  } else {
+ pipe->uxid = xfer->id;
  err = pipe->methods->start(xfer);
  if (err != USBD_IN_PROGRESS) {
  printf("usbd_start_next: error=%d\n", err);
Index: usbdivar.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v
retrieving revision 1.71
diff -u -p -r1.71 usbdivar.h
--- usbdivar.h 23 May 2016 11:31:12 -0000 1.71
+++ usbdivar.h 10 Mar 2017 12:00:59 -0000
@@ -176,6 +176,7 @@ struct usbd_pipe {
  SIMPLEQ_HEAD(, usbd_xfer) queue;
  LIST_ENTRY(usbd_pipe) next;
 
+ uint64_t uxid; /* xfer being currently transfered */
  struct usbd_xfer *intrxfer; /* used for repeating requests */
  char repeat;
  int interval;
@@ -194,7 +195,7 @@ struct usbd_xfer {
  u_int32_t timeout;
  usbd_status status;
  usbd_callback callback;
- volatile char done;
+ u_int64_t id;
 #ifdef DIAGNOSTIC
  u_int32_t busy_free;
 #define XFER_FREE 0x42555359

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fix multiple USB use-after-free

Martin Pieuchot
On 10/03/17(Fri) 15:27, Martin Pieuchot wrote:

> In polling mode, finished transfers are processed by the waiting thread.
> This happens inside usbd_dopoll().  That means it's unsafe to dereference
> ``xfer'' after calling it:
>
> 352: usbd_dopoll(pipe->device);
> 353: if (xfer->done)
> 354: break;
>
>
> When interrupts are enabled and a transfer times out it is unsafe to
> dereference ``xfer'' after being awaken.  That's because the aborting
> task calls wakeup(9) after executing the callback:
>
>
> 807: pipe->methods->done(xfer);
> 808: if (xfer->callback)
> 809: xfer->callback(xfer, xfer->priv, xfer->status);
>
> ...
>
> 821: if ((flags & USBD_SYNCHRONOUS) && !polling)
> 822: wakeup(xfer);
>
>
> These problems are inherent to the fact that the ``done'' flag is on
> the ``xfer'' descriptor.
>
> The diff below fixes the problem by using a unique ID to check if a
> submitted transfer is finished or not.  Since we know that the ``pipe''
> associated to a transfer wont disappear until it is closed, we can use
> it to store the current ID.
> When can also tsleep(9) on the address of the pipe instead of the address
> of the transfer.
>
> Note that with this change usbd_transfer() will no longer report all
> possible USB error messages.  But I'd say it's an improvement since
> not a single driver can handle them.

Updated diff on top of -current, I'd appreciate test reports and reviews
:)

Index: ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.200
diff -u -p -r1.200 ehci.c
--- ehci.c 15 May 2017 10:52:08 -0000 1.200
+++ ehci.c 15 May 2017 11:06:19 -0000
@@ -3350,7 +3350,6 @@ ehci_device_isoc_start(struct usbd_xfer
 
  TAILQ_INSERT_TAIL(&sc->sc_intrhead, ex, inext);
  xfer->status = USBD_IN_PROGRESS;
- xfer->done = 0;
  splx(s);
 
  return (USBD_IN_PROGRESS);
Index: usbdi.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.95
diff -u -p -r1.95 usbdi.c
--- usbdi.c 15 May 2017 10:52:08 -0000 1.95
+++ usbdi.c 18 May 2017 11:06:27 -0000
@@ -54,6 +54,8 @@ extern int usbdebug;
 #define DPRINTFN(n,x)
 #endif
 
+uint64_t usbd_xfer_id; /* unique ID per xfer */
+
 void usbd_request_async_cb(struct usbd_xfer *, void *, usbd_status);
 void usbd_start_next(struct usbd_pipe *pipe);
 usbd_status usbd_open_pipe_ival(struct usbd_interface *, u_int8_t, u_int8_t,
@@ -282,6 +284,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  struct usbd_bus *bus = pipe->device->bus;
  int polling = bus->use_polling;
  usbd_status err;
+ uint32_t timeout;
+ uint64_t uxid;
  int flags, s;
 
  if (usbd_is_dying(pipe->device))
@@ -293,8 +297,6 @@ usbd_transfer(struct usbd_xfer *xfer)
  if (usbdebug > 5)
  usbd_dump_queue(pipe);
 #endif
- xfer->done = 0;
-
  if (pipe->aborting)
  return (USBD_CANCELLED);
 
@@ -320,8 +322,12 @@ usbd_transfer(struct usbd_xfer *xfer)
  usb_syncmem(&xfer->dmabuf, 0, xfer->length,
     BUS_DMASYNC_PREREAD);
 
- err = pipe->methods->transfer(xfer);
+ /* We cannot dereference ``xfer'' after giving it to the HC. */
+ uxid = xfer->id;
+ flags = xfer->flags;
+ timeout = xfer->timeout;
 
+ err = pipe->methods->transfer(xfer);
  if (err != USBD_IN_PROGRESS && err != USBD_NORMAL_COMPLETION) {
  /* The transfer has not been queued, so free buffer. */
  if (xfer->rqflags & URQ_AUTO_DMABUF) {
@@ -330,50 +336,52 @@ usbd_transfer(struct usbd_xfer *xfer)
  }
  }
 
- if (!(xfer->flags & USBD_SYNCHRONOUS))
+ if (!(flags & USBD_SYNCHRONOUS) || (err != USBD_IN_PROGRESS))
  return (err);
 
  /* Sync transfer, wait for completion. */
- if (err != USBD_IN_PROGRESS)
- return (err);
-
  s = splusb();
  if (polling) {
  int timo;
 
- for (timo = xfer->timeout; timo >= 0; timo--) {
+ for (timo = timeout; timo >= 0; timo--) {
  usb_delay_ms(bus, 1);
  if (bus->dying) {
- xfer->status = USBD_IOERROR;
- usb_transfer_complete(xfer);
+ err = USBD_IOERROR;
  break;
  }
 
  usbd_dopoll(pipe->device);
- if (xfer->done)
+ if (pipe->uxid != uxid) {
+ err = USBD_NORMAL_COMPLETION;
  break;
+ }
  }
 
- if (timo < 0) {
- xfer->status = USBD_TIMEOUT;
- usb_transfer_complete(xfer);
- }
+ if (timo < 0)
+ err = USBD_TIMEOUT;
  } else {
- while (!xfer->done) {
- flags = PRIBIO|(xfer->flags & USBD_CATCH ? PCATCH : 0);
+ int error;
+
+ while (pipe->uxid == uxid) {
+ flags = PRIBIO | (flags & USBD_CATCH ? PCATCH : 0);
 
- err = tsleep(xfer, flags, "usbsyn", 0);
- if (err && !xfer->done) {
- usbd_abort_pipe(pipe);
- if (err == EINTR)
- xfer->status = USBD_INTERRUPTED;
+ error = tsleep(pipe, flags, "usbsyn", 0);
+ if (error && (pipe->uxid == uxid)) {
+ if (error == EINTR)
+ err = USBD_INTERRUPTED;
  else
- xfer->status = USBD_TIMEOUT;
+ err = USBD_TIMEOUT;
+ } else {
+ err = USBD_NORMAL_COMPLETION;
  }
  }
  }
+ if (err)
+ usbd_abort_pipe(pipe);
  splx(s);
- return (xfer->status);
+
+ return (err);
 }
 
 void *
@@ -414,6 +422,12 @@ usbd_alloc_xfer(struct usbd_device *dev)
  xfer = dev->bus->methods->allocx(dev->bus);
  if (xfer == NULL)
  return (NULL);
+
+ usbd_xfer_id++;
+ if (usbd_xfer_id == 0)
+ usbd_xfer_id++;
+
+ xfer->id = usbd_xfer_id;
 #ifdef DIAGNOSTIC
  xfer->busy_free = XFER_FREE;
 #endif
@@ -727,13 +741,6 @@ usb_transfer_complete(struct usbd_xfer *
  printf("%s: xfer=%p not on queue\n", __func__, xfer);
  return;
  }
-#endif
-
- /* XXXX */
- if (polling)
- pipe->running = 0;
-
-#ifdef DIAGNOSTIC
  if (xfer->actlen > xfer->length) {
  printf("%s: actlen > len %u > %u\n", __func__, xfer->actlen,
     xfer->length);
@@ -741,6 +748,9 @@ usb_transfer_complete(struct usbd_xfer *
  }
 #endif
 
+ if (polling)
+ pipe->running = 0;
+
  if (xfer->actlen != 0) {
  if (usbd_xfer_isread(xfer)) {
  usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
@@ -778,7 +788,7 @@ usb_transfer_complete(struct usbd_xfer *
  ++pipe->device->bus->stats.uds_requests
  [pipe->endpoint->edesc->bmAttributes & UE_XFERTYPE];
 
- xfer->done = 1;
+ pipe->uxid = 0;
  if (!xfer->status && xfer->actlen < xfer->length &&
     !(xfer->flags & USBD_SHORT_XFER_OK)) {
  DPRINTFN(-1,("usb_transfer_complete: short transfer %d<%d\n",
@@ -804,7 +814,7 @@ usb_transfer_complete(struct usbd_xfer *
  }
 
  if ((flags & USBD_SYNCHRONOUS) && !polling)
- wakeup(xfer);
+ wakeup(pipe);
 
  if (!pipe->repeat) {
  /* XXX should we stop the queue on all errors? */
@@ -838,6 +848,7 @@ usb_insert_transfer(struct usbd_xfer *xf
  if (pipe->running)
  err = USBD_IN_PROGRESS;
  else {
+ pipe->uxid = xfer->id;
  pipe->running = 1;
  err = USBD_NORMAL_COMPLETION;
  }
@@ -845,7 +856,6 @@ usb_insert_transfer(struct usbd_xfer *xf
  return (err);
 }
 
-/* Called at splusb() */
 void
 usbd_start_next(struct usbd_pipe *pipe)
 {
@@ -854,23 +864,13 @@ usbd_start_next(struct usbd_pipe *pipe)
 
  splsoftassert(IPL_SOFTUSB);
 
-#ifdef DIAGNOSTIC
- if (pipe == NULL) {
- printf("usbd_start_next: pipe == NULL\n");
- return;
- }
- if (pipe->methods == NULL || pipe->methods->start == NULL) {
- printf("usbd_start_next: pipe=%p no start method\n", pipe);
- return;
- }
-#endif
-
  /* Get next request in queue. */
  xfer = SIMPLEQ_FIRST(&pipe->queue);
  DPRINTFN(5, ("usbd_start_next: pipe=%p, xfer=%p\n", pipe, xfer));
  if (xfer == NULL) {
  pipe->running = 0;
  } else {
+ pipe->uxid = xfer->id;
  err = pipe->methods->start(xfer);
  if (err != USBD_IN_PROGRESS) {
  printf("usbd_start_next: error=%d\n", err);
Index: usbdivar.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v
retrieving revision 1.72
diff -u -p -r1.72 usbdivar.h
--- usbdivar.h 8 Apr 2017 02:57:25 -0000 1.72
+++ usbdivar.h 15 May 2017 11:06:19 -0000
@@ -177,6 +177,7 @@ struct usbd_pipe {
  SIMPLEQ_HEAD(, usbd_xfer) queue;
  LIST_ENTRY(usbd_pipe) next;
 
+ uint64_t uxid; /* xfer being currently transfered */
  struct usbd_xfer *intrxfer; /* used for repeating requests */
  char repeat;
  int interval;
@@ -195,7 +196,7 @@ struct usbd_xfer {
  u_int32_t timeout;
  usbd_status status;
  usbd_callback callback;
- volatile char done;
+ u_int64_t id;
 #ifdef DIAGNOSTIC
  u_int32_t busy_free;
 #define XFER_FREE 0x42555359

Loading...