Re: Fix multiple USB use-after-free
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 - 1.200
+++ ehci.c 15 May 2017 11:06:19 -
@@ -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 - 1.95
+++ usbdi.c 18 May 2017 11:06:27 -
@@ -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) {
-
Fix multiple USB use-after-free
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 - 1.197
+++ ehci.c 10 Mar 2017 12:00:57 -
@@ -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 - 1.89
+++ usbdi.c 10 Mar 2017 12:00:58 -
@@ -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
