[PATCH v2] gpib: fix use-after-free between iboffline() detach and in-flight I/O

[Pavitra Jha <jhapavitra98@gmail.com>]

iboffline() calls board->interface->detach(), which frees
board->private_data, with no guarantee that a read/write/command
ioctl already in progress on that board has finished touching it.

v1 of this fix took board->big_gpib_mutex around the detach() call.
That was wrong on two counts, both pointed out by Dave Penkler:

 1. iboffline() is reached from ibioctl() via the IBONL ioctl, which
    is dispatched while big_gpib_mutex is already held. Taking it
    again inside iboffline() self-deadlocks.

 2. IBRD/IBWRT/IBCMD explicitly drop big_gpib_mutex before calling
    into board->interface, because those calls can block for the
    duration of board->usec_timeout. So even without the deadlock,
    the mutex was never actually held during the in-flight callback
    it was meant to exclude.

Fix this by tracking in-flight callbacks directly instead of
overloading big_gpib_mutex. Add board->io_active, an atomic counter
incremented around the body of read_ioctl(), write_ioctl(), and
command_ioctl() (which is where board->interface is dereferenced,
via ibrd()/ibwrt()/ibcmd()), and board->io_drain_wait, a waitqueue
woken when the counter reaches zero.

iboffline() waits uninterruptibly on io_drain_wait before calling
detach(). big_gpib_mutex is already held at that point (via IBONL),
which blocks any *new* I/O ioctl from starting, so the wait only
drains whatever was already in flight; it does not need a timeout,
and adding one would just let detach() free private_data out from
under a callback that is still running.

gpib_unregister_driver() also calls iboffline(), but unlike the
IBONL path it holds no lock at all, so a fresh ioctl could still
race the detach()/board->interface=NULL sequence there. Take
big_gpib_mutex around that call site to close the same window.

The KASAN reproducer from v1 (kprobe on a driver read callback,
concurrent kfree from a detach kthread) demonstrates the class of
bug this closes: a blocking read callback dereferencing freed
private_data. It is reproduced against board->io_active rather than
the mutex in this version; behavior against the harness is
unchanged, since the harness only exercises the missing exclusion
between detach() and an in-flight read, not the specific mechanism
used to provide it.

Fixes: e6ab504633e4 ("staging: gpib: Destage gpib")
Cc: stable@vger.kernel.org
Signed-off-by: Pavitra Jha <jhapavitra98@gmail.com>
---
 drivers/gpib/common/gpib_os.c     | 21 +++++++++++++++++++++
 drivers/gpib/common/iblib.c       | 27 ++++++++++++++++-----------
 drivers/gpib/include/gpib_types.h |  9 +++++++++
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/gpib/common/gpib_os.c b/drivers/gpib/common/gpib_os.c
index 5909274dd..86921856f 100644
--- a/drivers/gpib/common/gpib_os.c
+++ b/drivers/gpib/common/gpib_os.c
@@ -912,6 +912,7 @@ static int read_ioctl(struct gpib_file_private *file_priv, struct gpib_board *bo
 	mutex_unlock(&file_priv->descriptors_mutex);
 
 	atomic_set(&desc->io_in_progress, 1);
+	atomic_inc(&board->io_active);
 
 	/* Read buffer loads till we fill the user supplied buffer */
 	while (remain > 0 && end_flag == 0) {
@@ -944,6 +945,8 @@ static int read_ioctl(struct gpib_file_private *file_priv, struct gpib_board *bo
 		retval = copy_to_user((void __user *)arg, &read_cmd, sizeof(read_cmd));
 
 	atomic_set(&desc->io_in_progress, 0);
+	atomic_dec(&board->io_active);
+	wake_up(&board->io_drain_wait);
 	atomic_dec(&desc->descriptor_busy);
 
 	wake_up_interruptible(&board->wait);
@@ -1003,6 +1006,7 @@ static int command_ioctl(struct gpib_file_private *file_priv,
 	 */
 
 	atomic_set(&desc->io_in_progress, 1);
+	atomic_inc(&board->io_active);
 
 	do {
 		fault = copy_from_user(board->buffer, userbuf, (board->buffer_length < remain) ?
@@ -1038,6 +1042,8 @@ static int command_ioctl(struct gpib_file_private *file_priv,
 	 */
 	if (!no_clear_io_in_prog || fault)
 		atomic_set(&desc->io_in_progress, 0);
+	atomic_dec(&board->io_active);
+	wake_up(&board->io_drain_wait);
 	atomic_dec(&desc->descriptor_busy);
 
 	wake_up_interruptible(&board->wait);
@@ -1085,6 +1091,7 @@ static int write_ioctl(struct gpib_file_private *file_priv, struct gpib_board *b
 	mutex_unlock(&file_priv->descriptors_mutex);
 
 	atomic_set(&desc->io_in_progress, 1);
+	atomic_inc(&board->io_active);
 
 	/* Write buffer loads till we empty the user supplied buffer */
 	while (remain > 0) {
@@ -1118,6 +1125,8 @@ static int write_ioctl(struct gpib_file_private *file_priv, struct gpib_board *b
 		fault = copy_to_user((void __user *)arg, &write_cmd, sizeof(write_cmd));
 
 	atomic_set(&desc->io_in_progress, 0);
+	atomic_dec(&board->io_active);
+	wake_up(&board->io_drain_wait);
 	atomic_dec(&desc->descriptor_busy);
 
 	wake_up_interruptible(&board->wait);
@@ -2115,8 +2124,18 @@ void gpib_unregister_driver(struct gpib_interface *interface)
 			if (board->use_count > 0)
 				pr_warn("gpib: Warning: deregistered interface %s in use\n",
 					interface->name);
+			/*
+			 * Unlike the IBONL ioctl path, nothing else holds
+			 * big_gpib_mutex here, so a fresh ioctl could race
+			 * this teardown and dispatch into board->interface
+			 * right as it becomes NULL below. Hold the mutex
+			 * across iboffline() and clearing board->interface
+			 * so ibioctl() cannot enter until this is done.
+			 */
+			mutex_lock(&board->big_gpib_mutex);
 			iboffline(board);
 			board->interface = NULL;
+			mutex_unlock(&board->big_gpib_mutex);
 		}
 	}
 	for (list_ptr = registered_drivers.next; list_ptr != &registered_drivers;) {
@@ -2149,6 +2168,8 @@ void init_gpib_board(struct gpib_board *board)
 	init_waitqueue_head(&board->wait);
 	mutex_init(&board->user_mutex);
 	mutex_init(&board->big_gpib_mutex);
+	atomic_set(&board->io_active, 0);
+	init_waitqueue_head(&board->io_drain_wait);
 	board->locking_pid = 0;
 	spin_lock_init(&board->locking_pid_spinlock);
 	spin_lock_init(&board->spinlock);
diff --git a/drivers/gpib/common/iblib.c b/drivers/gpib/common/iblib.c
index 07a30d520..0de6ca50c 100644
--- a/drivers/gpib/common/iblib.c
+++ b/drivers/gpib/common/iblib.c
@@ -257,22 +257,27 @@ int iboffline(struct gpib_board *board)
 	}
 
 	/*
-	 * Acquire big_gpib_mutex before calling detach() to prevent a
-	 * use-after-free race. I/O callbacks (read/write/command) hold
-	 * big_gpib_mutex while caching board->private_data on their stack.
-	 * Without this lock, iboffline() can kfree(board->private_data)
-	 * inside detach() while an I/O callback is still running and holds
-	 * a stale pointer to the freed memory.
+	 * iboffline() is always called with board->big_gpib_mutex already
+	 * held (via the IBONL ioctl), so it cannot take that mutex itself.
+	 * That mutex is also dropped by IBRD/IBWRT/IBCMD before they call
+	 * into board->interface, since those calls can block for a long
+	 * time, so it never actually excludes an in-flight read/write/
+	 * command callback in the first place.
 	 *
-	 * Affected board drivers: cb7210, ines_gpib, tnt4882 (all delegate
-	 * to nec7210_read/pio_read which blocks in wait_event_interruptible
-	 * for up to board->usec_timeout microseconds while holding priv).
+	 * board->io_active counts callbacks currently executing inside
+	 * board->interface. Wait here, uninterruptibly and without a
+	 * timeout, for it to reach zero before calling detach(). Since
+	 * big_gpib_mutex is held, no *new* I/O ioctl can start while we
+	 * wait, so this only drains whatever was already in flight.
+	 * Returning early would let detach() free board->private_data
+	 * while that in-flight callback is still dereferencing it, which
+	 * is the use-after-free this is closing.
 	 */
-	mutex_lock(&board->big_gpib_mutex);
+	wait_event(board->io_drain_wait, atomic_read(&board->io_active) == 0);
+
 	board->interface->detach(board);
 	gpib_deallocate_board(board);
 	board->online = 0;
-	mutex_unlock(&board->big_gpib_mutex);
 	dev_dbg(board->gpib_dev, "board offline\n");
 
 	return 0;
diff --git a/drivers/gpib/include/gpib_types.h b/drivers/gpib/include/gpib_types.h
index 28b73157f..1c1f29c12 100644
--- a/drivers/gpib/include/gpib_types.h
+++ b/drivers/gpib/include/gpib_types.h
@@ -288,6 +288,15 @@ struct gpib_board {
 	 * store additional variables for this board
 	 */
 	void *private_data;
+	/*
+	 * Counts read/write/command callbacks currently executing in
+	 * board->interface. iboffline() waits for this to reach zero
+	 * before calling detach(), so its teardown of private_data
+	 * cannot race an in-flight callback still dereferencing it.
+	 */
+	atomic_t io_active;
+	/* Woken when io_active reaches zero, so iboffline() can wait for drain. */
+	wait_queue_head_t io_drain_wait;
 	/* Number of open file descriptors using this board */
 	unsigned int use_count;
 	/* list of open devices connected to this board */
-- 
2.53.0