[PATCH] tty: serialize device registration and removal

[PATCH] tty: serialize device registration and removal

[Chengfeng Ye]

tty_cdev_add() stores a newly allocated cdev in driver->cdevs[index]
before initializing and registering it. tty_unregister_device() reads and
deletes the same pointer without serialization.

The race can proceed as follows:

  registration                     removal
  ------------                     -------
  driver->cdevs[index] = cdev
                                   cdev_del(driver->cdevs[index])
                                   kobject_put() frees cdev
  driver->cdevs[index]->ops = &tty_fops

The final write accesses freed memory. Concurrent registration can also
replace the slot while another caller is still using its cdev.

KASAN reported:

  BUG: KASAN: slab-use-after-free in tty_cdev_add+0x5c7/0x670
  Write of size 8 at addr ffff88810b298c48

  Call Trace:
   tty_cdev_add+0x5c7/0x670
   tty_register_device_attr+0x458/0x810
   gsm_activate_mux+0x105/0x2e0
   gsmld_ioctl+0x92f/0x14d0

  Allocated by task 111:
   cdev_alloc+0x99/0x130
   tty_cdev_add+0x93/0x670

  Freed by task 110:
   cdev_dynamic_release+0x38/0x50
   kobject_put+0x14d/0x280
   tty_unregister_device+0x1b7/0x3c0

Add a per-driver mutex and hold it across complete device registration and
removal. This prevents either transition from replacing or freeing a cdev
until the other has finished.

Fixes: a3a10ce3429e ("Avoid usb reset crashes by making tty_io cdevs truly dynamic")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 drivers/tty/tty_io.c       | 13 ++++++++++---
 include/linux/tty_driver.h |  3 +++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6b283fd03ff8..926a9b586c7d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3185,7 +3185,8 @@ static int tty_cdev_add(struct tty_driver *driver, dev_t dev,
  * that bit is not set, this function should not be called by a tty
  * driver.
  *
- * Locking: ??
+ * Locking: device registration and removal are serialized by
+ * @driver->device_mutex.
  *
  * Return: A pointer to the struct device for this tty device (or
  * ERR_PTR(-EFOO) on error).
@@ -3217,7 +3218,8 @@ static void tty_device_create_release(struct device *dev)
  * tty driver's flags have the %TTY_DRIVER_DYNAMIC_DEV bit set. If that bit is
  * not set, this function should not be called by a tty driver.
  *
- * Locking: ??
+ * Locking: device registration and removal are serialized by
+ * @driver->device_mutex.
  *
  * Return: A pointer to the struct device for this tty device (or
  * ERR_PTR(-EFOO) on error).
@@ -3257,6 +3259,7 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	dev_set_drvdata(dev, drvdata);
 
 	dev_set_uevent_suppress(dev, 1);
+	guard(mutex)(&driver->device_mutex);
 
 	retval = device_register(dev);
 	if (retval)
@@ -3300,10 +3303,13 @@ EXPORT_SYMBOL_GPL(tty_register_device_attr);
  * If a tty device is registered with a call to tty_register_device() then
  * this function must be called when the tty device is gone.
  *
- * Locking: ??
+ * Locking: device registration and removal are serialized by
+ * @driver->device_mutex.
  */
 void tty_unregister_device(struct tty_driver *driver, unsigned index)
 {
+	guard(mutex)(&driver->device_mutex);
+
 	device_destroy(&tty_class, MKDEV(driver->major, driver->minor_start) + index);
 	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
 		cdev_del(driver->cdevs[index]);
@@ -3338,6 +3344,7 @@ struct tty_driver *__tty_alloc_driver(unsigned int lines, struct module *owner,
 		return ERR_PTR(-ENOMEM);
 
 	kref_init(&driver->kref);
+	mutex_init(&driver->device_mutex);
 	driver->num = lines;
 	driver->owner = owner;
 	driver->flags = flags;
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 1f2896e56e77..bf8c9d8ec698 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -7,6 +7,7 @@
 #include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/cdev.h>
+#include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/termios.h>
 #include <linux/seq_file.h>
@@ -497,6 +498,7 @@ struct tty_operations {
  * @kref: reference counting. Reaching zero frees all the internals and the
  *	  driver.
  * @cdevs: allocated/registered character /dev devices
+ * @device_mutex: serializes device registration and removal
  * @owner: modules owning this driver. Used drivers cannot be rmmod'ed.
  *	   Automatically set by tty_alloc_driver().
  * @driver_name: name of the driver used in /proc/tty
@@ -532,6 +534,7 @@ struct tty_operations {
 struct tty_driver {
 	struct kref kref;
 	struct cdev **cdevs;
+	struct mutex device_mutex;
 	struct module	*owner;
 	const char	*driver_name;
 	const char	*name;
-- 
2.43.0