[PATCH] tty: n_gsm: serialize mux configuration

[PATCH] tty: n_gsm: serialize mux configuration

[Artem Novikov]

From: Artem Novikov <naa@amicon.ru>

Concurrent GSMIOC_SETCONF calls can both decide to restart the same mux.
gsm_cleanup_mux() releases gsm->mutex before gsm_config() calls
gsm_activate_mux(), so cleanup and activation from different calls can
overlap. The activation paths can register the same gsmtty minors, and
registration rollback can race with another registration or cleanup.
GSMIOC_SETCONF_EXT can restart the mux through the same paths.

A 5.10.246-syzkaller report shows a NULL dereference in the unregister
path:

 KASAN: null-ptr-deref in range [0x0000000000000060-0x0000000000000067]
 RIP: 0010:cdev_del+0x22/0x90 fs/char_dev.c:596
 Call Trace:
  tty_unregister_device drivers/tty/tty_io.c:3290 [inline]
  tty_unregister_device+0x117/0x1c0 drivers/tty/tty_io.c:3285
  gsm_register_devices drivers/tty/n_gsm.c:496 [inline]
  gsm_activate_mux drivers/tty/n_gsm.c:2286 [inline]
  gsm_config drivers/tty/n_gsm.c:2517 [inline]
  gsmld_ioctl+0x9b2/0xff0 drivers/tty/n_gsm.c:2829
  tty_ioctl+0x86a/0x16d0 drivers/tty/tty_io.c:2757
  vfs_ioctl fs/ioctl.c:48 [inline]
  __do_sys_ioctl fs/ioctl.c:753 [inline]
  __se_sys_ioctl fs/ioctl.c:739 [inline]
  __x64_sys_ioctl+0x19a/0x210 fs/ioctl.c:739
  do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
  entry_SYSCALL_64_after_hwframe+0x67/0xd1

Current tty_unregister_device() checks for a NULL cdev. The overlapping
registration and cleanup operations in n_gsm remain unsynchronized.

Serialize the complete configuration transaction with a per-mux mutex.
Use the same mutex for GSMIOC_SETCONF_EXT, which can also restart the mux,
and for the corresponding configuration snapshots. Keep userspace copies
outside the critical sections. Use a separate mutex because
gsm_cleanup_mux() already acquires gsm->mutex.

Leave receive, transmit, timer and workqueue callbacks outside the new
lock so that control-channel shutdown can still make progress. Concurrent
configuration requests for one mux now wait for its reconfiguration to
finish; independent muxes retain separate locks.

Fixes: 01aecd917114 ("tty: n_gsm: fix tty registration before control channel open")
Cc: stable@vger.kernel.org
Signed-off-by: Artem Novikov <naa@amicon.ru>
---
 drivers/tty/n_gsm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e55779ff0af2bb6328cf5f0ec70497107f1462e8..26e34e79b034a19e1b5dc1f4a87d56a74e884d56 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -273,6 +273,7 @@ struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	struct mutex mutex;
+	struct mutex config_mutex;	/* Serializes mux configuration ioctls */
 	unsigned int num;
 	struct kref ref;
 
@@ -3219,6 +3220,7 @@ static void gsm_free_mux(struct gsm_mux *gsm)
 			break;
 		}
 	}
+	mutex_destroy(&gsm->config_mutex);
 	mutex_destroy(&gsm->mutex);
 	kfree(gsm->txframe);
 	kfree(gsm->buf);
@@ -3290,6 +3292,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	}
 	spin_lock_init(&gsm->lock);
 	mutex_init(&gsm->mutex);
+	mutex_init(&gsm->config_mutex);
 	kref_init(&gsm->ref);
 	INIT_LIST_HEAD(&gsm->tx_ctrl_list);
 	INIT_LIST_HEAD(&gsm->tx_data_list);
@@ -3328,6 +3331,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	}
 	spin_unlock(&gsm_mux_lock);
 	if (i == MAX_MUX) {
+		mutex_destroy(&gsm->config_mutex);
 		mutex_destroy(&gsm->mutex);
 		kfree(gsm->txframe);
 		kfree(gsm->buf);
@@ -3341,6 +3345,8 @@ static struct gsm_mux *gsm_alloc_mux(void)
 static void gsm_copy_config_values(struct gsm_mux *gsm,
 				   struct gsm_config *c)
 {
+	guard(mutex)(&gsm->config_mutex);
+
 	memset(c, 0, sizeof(*c));
 	c->adaption = gsm->adaption;
 	c->encapsulation = gsm->encoding;
@@ -3384,6 +3390,10 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 		return -EINVAL;
 	if (c->i == 0 || c->i > 2)	/* UIH and UI only */
 		return -EINVAL;
+
+	/* gsm_cleanup_mux() takes gsm->mutex, so use a separate lock. */
+	guard(mutex)(&gsm->config_mutex);
+
 	/*
 	 * See what is needed for reconfiguration
 	 */
@@ -3451,6 +3461,8 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 static void gsm_copy_config_ext_values(struct gsm_mux *gsm,
 				       struct gsm_config_ext *ce)
 {
+	guard(mutex)(&gsm->config_mutex);
+
 	memset(ce, 0, sizeof(*ce));
 	ce->wait_config = gsm->wait_config ? 1 : 0;
 	ce->keep_alive = gsm->keep_alive;
@@ -3471,6 +3483,8 @@ static int gsm_config_ext(struct gsm_mux *gsm, struct gsm_config_ext *ce)
 	if (ce->flags & ~GSM_FL_RESTART)
 		return -EINVAL;
 
+	guard(mutex)(&gsm->config_mutex);
+
 	/* Requires care */
 	if (ce->flags & GSM_FL_RESTART)
 		need_restart = true;

base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb