From b17d53677f18cad70f07783f1d4ab96d4ebe5f40 Mon Sep 17 00:00:00 2001
From: Martin Pieuchot <mpi@cvs.openbsd.org>
Date: Mon, 6 Mar 2017 09:04:59 +0000
Subject: It is unsafe to dereference ``xfer'' after calling the callback as it
 might free it.

Prevent a use-after-free in various aynchronous cases.  Found while
looking at another user-after-free pointed out by ehrhardt@.
---
 sys/dev/usb/usbdi.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

(limited to 'sys/dev/usb/usbdi.c')

diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
index 2d25dd7bac3..b0d4946a573 100644
--- a/sys/dev/usb/usbdi.c
+++ b/sys/dev/usb/usbdi.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: usbdi.c,v 1.85 2016/10/04 14:12:05 mpi Exp $ */
+/*	$OpenBSD: usbdi.c,v 1.86 2017/03/06 09:04:58 mpi Exp $ */
 /*	$NetBSD: usbdi.c,v 1.103 2002/09/27 15:37:38 provos Exp $	*/
 /*	$FreeBSD: src/sys/dev/usb/usbdi.c,v 1.28 1999/11/17 22:33:49 n_hibma Exp $	*/
 
@@ -696,12 +696,13 @@ void
 usb_transfer_complete(struct usbd_xfer *xfer)
 {
 	struct usbd_pipe *pipe = xfer->pipe;
-	int polling;
+	int polling = pipe->device->bus->use_polling;
+	int status, flags;
 
 	SPLUSBCHECK;
 
 	DPRINTFN(5, ("usb_transfer_complete: pipe=%p xfer=%p status=%d "
-		     "actlen=%d\n", pipe, xfer, xfer->status, xfer->actlen));
+		     "actlen=%d\n", pipe, xfer, status, xfer->actlen));
 #ifdef DIAGNOSTIC
 	if (xfer->busy_free != XFER_ONQU) {
 		printf("%s: xfer=%p not on queue\n", __func__, xfer);
@@ -715,7 +716,6 @@ usb_transfer_complete(struct usbd_xfer *xfer)
 		return;
 	}
 #endif
-	polling = pipe->device->bus->use_polling;
 	/* XXXX */
 	if (polling)
 		pipe->running = 0;
@@ -773,6 +773,13 @@ usb_transfer_complete(struct usbd_xfer *xfer)
 		xfer->status = USBD_SHORT_XFER;
 	}
 
+	/*
+	 * We cannot dereference ``xfer'' after calling the callback as
+	 * it might free it.
+	 */
+	status = xfer->status;
+	flags = xfer->flags;
+
 	if (pipe->repeat) {
 		if (xfer->callback)
 			xfer->callback(xfer, xfer->priv, xfer->status);
@@ -789,17 +796,16 @@ usb_transfer_complete(struct usbd_xfer *xfer)
 	 * a new transfer as it will more likely results in the same
 	 * error.
 	 */
-	if (xfer->status == USBD_IOERROR)
+	if (status == USBD_IOERROR)
 		pipe->repeat = 0;
 
-	if ((xfer->flags & USBD_SYNCHRONOUS) && !polling)
+	if ((flags & USBD_SYNCHRONOUS) && !polling)
 		wakeup(xfer);
 
 	if (!pipe->repeat) {
 		/* XXX should we stop the queue on all errors? */
-		if ((xfer->status == USBD_CANCELLED ||
-		     xfer->status == USBD_IOERROR ||
-		     xfer->status == USBD_TIMEOUT) &&
+		if ((status == USBD_CANCELLED || status == USBD_IOERROR ||
+		     status == USBD_TIMEOUT) &&
 		    pipe->iface != NULL)		/* not control pipe */
 			pipe->running = 0;
 		else
-- 
cgit v1.2.3