[PATCH v5 0/2] gpiolib: don't allow user-space to crash the kernel with hot-unplugs

[PATCH v5 0/2] gpiolib: don't allow user-space to crash the kernel with hot-unplugs

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This is (hopefully) the final iteration of the changes that aim at fixing
the situation in which the user-space can provoke a NULL-pointer derefence
in the kernel when a GPIO device that's in use by user-space is removed.

v4 -> v5:
- try to acquire the semaphore for reading and bail out of syscall callbacks
  immediately in case of lock contention

v3 -> v4:
- use function typedefs to make code cleaner
- add a blank line after down_write()

v2 -> v3:
- drop the helper variable in patch 1/2 as we won't be using it in 2/2
- refactor patch 2/2 to use locking wrappers around the syscall callbacks

v1 -> v2:
- add missing gdev->chip checks in patch 1/2
- add a second patch that protects the structures that can be accessed
  by user-space calls against concurrent removal

Bartosz Golaszewski (2):
  gpiolib: cdev: fix NULL-pointer dereferences
  gpiolib: protect the GPIO device against being dropped while in use by
    user-space

 drivers/gpio/gpiolib-cdev.c | 193 ++++++++++++++++++++++++++++++++----
 drivers/gpio/gpiolib.c      |   4 +
 drivers/gpio/gpiolib.h      |   5 +
 3 files changed, 180 insertions(+), 22 deletions(-)

-- 
2.37.2

[PATCH v5 1/2] gpiolib: cdev: fix NULL-pointer dereferences

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There are several places where we can crash the kernel by requesting
lines, unbinding the GPIO device, then calling any of the system calls
relevant to the GPIO character device's annonymous file descriptors:
ioctl(), read(), poll().

While I observed it with the GPIO simulator, it will also happen for any
of the GPIO devices that can be hot-unplugged - for instance any HID GPIO
expander (e.g. CP2112).

This affects both v1 and v2 uAPI.

This fixes it partially by checking if gdev->chip is not NULL but it
doesn't entirely remedy the situation as we still have a race condition
in which another thread can remove the device after the check.

Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
Fixes: 7b8e00d98168 ("gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-cdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0cb6b468f364..911d91668903 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -201,6 +201,9 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
 	unsigned int i;
 	int ret;
 
+	if (!lh->gdev->chip)
+		return -ENODEV;
+
 	switch (cmd) {
 	case GPIOHANDLE_GET_LINE_VALUES_IOCTL:
 		/* NOTE: It's okay to read values of output lines */
@@ -1384,6 +1387,9 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
 	struct linereq *lr = file->private_data;
 	void __user *ip = (void __user *)arg;
 
+	if (!lr->gdev->chip)
+		return -ENODEV;
+
 	switch (cmd) {
 	case GPIO_V2_LINE_GET_VALUES_IOCTL:
 		return linereq_get_values(lr, ip);
@@ -1410,6 +1416,9 @@ static __poll_t linereq_poll(struct file *file,
 	struct linereq *lr = file->private_data;
 	__poll_t events = 0;
 
+	if (!lr->gdev->chip)
+		return 0;
+
 	poll_wait(file, &lr->wait, wait);
 
 	if (!kfifo_is_empty_spinlocked_noirqsave(&lr->events,
@@ -1429,6 +1438,9 @@ static ssize_t linereq_read(struct file *file,
 	ssize_t bytes_read = 0;
 	int ret;
 
+	if (!lr->gdev->chip)
+		return -ENODEV;
+
 	if (count < sizeof(le))
 		return -EINVAL;
 
@@ -1716,6 +1728,9 @@ static __poll_t lineevent_poll(struct file *file,
 	struct lineevent_state *le = file->private_data;
 	__poll_t events = 0;
 
+	if (!le->gdev->chip)
+		return 0;
+
 	poll_wait(file, &le->wait, wait);
 
 	if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock))
@@ -1740,6 +1755,9 @@ static ssize_t lineevent_read(struct file *file,
 	ssize_t ge_size;
 	int ret;
 
+	if (!le->gdev->chip)
+		return -ENODEV;
+
 	/*
 	 * When compatible system call is being used the struct gpioevent_data,
 	 * in case of at least ia32, has different size due to the alignment
@@ -1821,6 +1839,9 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
 	void __user *ip = (void __user *)arg;
 	struct gpiohandle_data ghd;
 
+	if (!le->gdev->chip)
+		return -ENODEV;
+
 	/*
 	 * We can get the value for an event line but not set it,
 	 * because it is input by definition.
@@ -2407,6 +2428,9 @@ static __poll_t lineinfo_watch_poll(struct file *file,
 	struct gpio_chardev_data *cdev = file->private_data;
 	__poll_t events = 0;
 
+	if (!cdev->gdev->chip)
+		return 0;
+
 	poll_wait(file, &cdev->wait, pollt);
 
 	if (!kfifo_is_empty_spinlocked_noirqsave(&cdev->events,
@@ -2425,6 +2449,9 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 	int ret;
 	size_t event_size;
 
+	if (!cdev->gdev->chip)
+		return -ENODEV;
+
 #ifndef CONFIG_GPIO_CDEV_V1
 	event_size = sizeof(struct gpio_v2_line_info_changed);
 	if (count < event_size)
-- 
2.37.2

[PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

While any of the GPIO cdev syscalls is in progress, the kernel can call
gpiochip_remove() (for instance, when a USB GPIO expander is disconnected)
which will set gdev->chip to NULL after which any subsequent access will
cause a crash.

To avoid that: use an RW-semaphore in which the syscalls take it for
reading (so that we don't needlessly prohibit the user-space from calling
syscalls simultaneously) while gpiochip_remove() takes it for writing so
that it can only happen once all syscalls return.

Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
Fixes: 7b8e00d98168 ("gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Kent Gibson <warthog618@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-cdev.c | 166 +++++++++++++++++++++++++++++++-----
 drivers/gpio/gpiolib.c      |   4 +
 drivers/gpio/gpiolib.h      |   5 ++
 3 files changed, 153 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 911d91668903..18c5e70ee7de 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -84,6 +84,53 @@ struct linehandle_state {
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
+typedef __poll_t (*poll_fn)(struct file *, struct poll_table_struct *);
+typedef long (*ioctl_fn)(struct file *, unsigned int, unsigned long);
+typedef ssize_t (*read_fn)(struct file *, char __user *,
+			   size_t count, loff_t *);
+
+static __poll_t call_poll_locked(struct file *file,
+				 struct poll_table_struct *wait,
+				 struct gpio_device *gdev, poll_fn func)
+{
+	__poll_t ret;
+
+	if (!down_read_trylock(&gdev->sem))
+		return 0;
+	ret = func(file, wait);
+	up_read(&gdev->sem);
+
+	return ret;
+}
+
+static long call_ioctl_locked(struct file *file, unsigned int cmd,
+			      unsigned long arg, struct gpio_device *gdev,
+			      ioctl_fn func)
+{
+	long ret;
+
+	if (!down_read_trylock(&gdev->sem))
+		return -ENODEV;
+	ret = func(file, cmd, arg);
+	up_read(&gdev->sem);
+
+	return ret;
+}
+
+static ssize_t call_read_locked(struct file *file, char __user *buf,
+				size_t count, loff_t *f_ps,
+				struct gpio_device *gdev, read_fn func)
+{
+	ssize_t ret;
+
+	if (!down_read_trylock(&gdev->sem))
+		return -ENODEV;
+	ret = func(file, buf, count, f_ps);
+	up_read(&gdev->sem);
+
+	return ret;
+}
+
 static int linehandle_validate_flags(u32 flags)
 {
 	/* Return an error if an unknown flag is set */
@@ -191,8 +238,8 @@ static long linehandle_set_config(struct linehandle_state *lh,
 	return 0;
 }
 
-static long linehandle_ioctl(struct file *file, unsigned int cmd,
-			     unsigned long arg)
+static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
+				      unsigned long arg)
 {
 	struct linehandle_state *lh = file->private_data;
 	void __user *ip = (void __user *)arg;
@@ -250,6 +297,15 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
 	}
 }
 
+static long linehandle_ioctl(struct file *file, unsigned int cmd,
+			     unsigned long arg)
+{
+	struct linehandle_state *lh = file->private_data;
+
+	return call_ioctl_locked(file, cmd, arg, lh->gdev,
+				 linehandle_ioctl_unlocked);
+}
+
 #ifdef CONFIG_COMPAT
 static long linehandle_ioctl_compat(struct file *file, unsigned int cmd,
 				    unsigned long arg)
@@ -1381,8 +1437,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
 	return ret;
 }
 
-static long linereq_ioctl(struct file *file, unsigned int cmd,
-			  unsigned long arg)
+static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
+				   unsigned long arg)
 {
 	struct linereq *lr = file->private_data;
 	void __user *ip = (void __user *)arg;
@@ -1402,6 +1458,15 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
 	}
 }
 
+static long linereq_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
+{
+	struct linereq *lr = file->private_data;
+
+	return call_ioctl_locked(file, cmd, arg, lr->gdev,
+				 linereq_ioctl_unlocked);
+}
+
 #ifdef CONFIG_COMPAT
 static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
 				 unsigned long arg)
@@ -1410,8 +1475,8 @@ static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
 }
 #endif
 
-static __poll_t linereq_poll(struct file *file,
-			    struct poll_table_struct *wait)
+static __poll_t linereq_poll_unlocked(struct file *file,
+				      struct poll_table_struct *wait)
 {
 	struct linereq *lr = file->private_data;
 	__poll_t events = 0;
@@ -1428,10 +1493,16 @@ static __poll_t linereq_poll(struct file *file,
 	return events;
 }
 
-static ssize_t linereq_read(struct file *file,
-			    char __user *buf,
-			    size_t count,
-			    loff_t *f_ps)
+static __poll_t linereq_poll(struct file *file,
+			     struct poll_table_struct *wait)
+{
+	struct linereq *lr = file->private_data;
+
+	return call_poll_locked(file, wait, lr->gdev, linereq_poll_unlocked);
+}
+
+static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
+				     size_t count, loff_t *f_ps)
 {
 	struct linereq *lr = file->private_data;
 	struct gpio_v2_line_event le;
@@ -1485,6 +1556,15 @@ static ssize_t linereq_read(struct file *file,
 	return bytes_read;
 }
 
+static ssize_t linereq_read(struct file *file, char __user *buf,
+			    size_t count, loff_t *f_ps)
+{
+	struct linereq *lr = file->private_data;
+
+	return call_read_locked(file, buf, count, f_ps, lr->gdev,
+				linereq_read_unlocked);
+}
+
 static void linereq_free(struct linereq *lr)
 {
 	unsigned int i;
@@ -1722,8 +1802,8 @@ struct lineevent_state {
 	(GPIOEVENT_REQUEST_RISING_EDGE | \
 	GPIOEVENT_REQUEST_FALLING_EDGE)
 
-static __poll_t lineevent_poll(struct file *file,
-			       struct poll_table_struct *wait)
+static __poll_t lineevent_poll_unlocked(struct file *file,
+					struct poll_table_struct *wait)
 {
 	struct lineevent_state *le = file->private_data;
 	__poll_t events = 0;
@@ -1739,15 +1819,21 @@ static __poll_t lineevent_poll(struct file *file,
 	return events;
 }
 
+static __poll_t lineevent_poll(struct file *file,
+			       struct poll_table_struct *wait)
+{
+	struct lineevent_state *le = file->private_data;
+
+	return call_poll_locked(file, wait, le->gdev, lineevent_poll_unlocked);
+}
+
 struct compat_gpioeevent_data {
 	compat_u64	timestamp;
 	u32		id;
 };
 
-static ssize_t lineevent_read(struct file *file,
-			      char __user *buf,
-			      size_t count,
-			      loff_t *f_ps)
+static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
+				       size_t count, loff_t *f_ps)
 {
 	struct lineevent_state *le = file->private_data;
 	struct gpioevent_data ge;
@@ -1815,6 +1901,15 @@ static ssize_t lineevent_read(struct file *file,
 	return bytes_read;
 }
 
+static ssize_t lineevent_read(struct file *file, char __user *buf,
+			      size_t count, loff_t *f_ps)
+{
+	struct lineevent_state *le = file->private_data;
+
+	return call_read_locked(file, buf, count, f_ps, le->gdev,
+				lineevent_read_unlocked);
+}
+
 static void lineevent_free(struct lineevent_state *le)
 {
 	if (le->irq)
@@ -1832,8 +1927,8 @@ static int lineevent_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static long lineevent_ioctl(struct file *file, unsigned int cmd,
-			    unsigned long arg)
+static long lineevent_ioctl_unlocked(struct file *file, unsigned int cmd,
+				     unsigned long arg)
 {
 	struct lineevent_state *le = file->private_data;
 	void __user *ip = (void __user *)arg;
@@ -1864,6 +1959,15 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
 	return -EINVAL;
 }
 
+static long lineevent_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct lineevent_state *le = file->private_data;
+
+	return call_ioctl_locked(file, cmd, arg, le->gdev,
+				 lineevent_ioctl_unlocked);
+}
+
 #ifdef CONFIG_COMPAT
 static long lineevent_ioctl_compat(struct file *file, unsigned int cmd,
 				   unsigned long arg)
@@ -2422,8 +2526,8 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static __poll_t lineinfo_watch_poll(struct file *file,
-				    struct poll_table_struct *pollt)
+static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
+					     struct poll_table_struct *pollt)
 {
 	struct gpio_chardev_data *cdev = file->private_data;
 	__poll_t events = 0;
@@ -2440,8 +2544,17 @@ static __poll_t lineinfo_watch_poll(struct file *file,
 	return events;
 }
 
-static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
-				   size_t count, loff_t *off)
+static __poll_t lineinfo_watch_poll(struct file *file,
+				    struct poll_table_struct *pollt)
+{
+	struct gpio_chardev_data *cdev = file->private_data;
+
+	return call_poll_locked(file, pollt, cdev->gdev,
+				lineinfo_watch_poll_unlocked);
+}
+
+static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
+					    size_t count, loff_t *off)
 {
 	struct gpio_chardev_data *cdev = file->private_data;
 	struct gpio_v2_line_info_changed event;
@@ -2519,6 +2632,15 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 	return bytes_read;
 }
 
+static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
+				   size_t count, loff_t *off)
+{
+	struct gpio_chardev_data *cdev = file->private_data;
+
+	return call_read_locked(file, buf, count, off, cdev->gdev,
+				lineinfo_watch_read_unlocked);
+}
+
 /**
  * gpio_chrdev_open() - open the chardev for ioctl operations
  * @inode: inode for this chardev
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4756ea08894f..e0e73bd756ca 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -731,6 +731,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->notifier);
+	init_rwsem(&gdev->sem);
 
 #ifdef CONFIG_PINCTRL
 	INIT_LIST_HEAD(&gdev->pin_ranges);
@@ -865,6 +866,8 @@ void gpiochip_remove(struct gpio_chip *gc)
 	unsigned long	flags;
 	unsigned int	i;
 
+	down_write(&gdev->sem);
+
 	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
 	gpiochip_sysfs_unregister(gdev);
 	gpiochip_free_hogs(gc);
@@ -899,6 +902,7 @@ void gpiochip_remove(struct gpio_chip *gc)
 	 * gone.
 	 */
 	gcdev_unregister(gdev);
+	up_write(&gdev->sem);
 	put_device(&gdev->dev);
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d900ecdbac46..9ad68a0adf4a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/cdev.h>
+#include <linux/rwsem.h>
 
 #define GPIOCHIP_NAME	"gpiochip"
 
@@ -39,6 +40,9 @@
  * @list: links gpio_device:s together for traversal
  * @notifier: used to notify subscribers about lines being requested, released
  *            or reconfigured
+ * @sem: protects the structure from a NULL-pointer dereference of @chip by
+ *       user-space operations when the device gets unregistered during
+ *       a hot-unplug event
  * @pin_ranges: range of pins served by the GPIO driver
  *
  * This state container holds most of the runtime variable data
@@ -60,6 +64,7 @@ struct gpio_device {
 	void			*data;
 	struct list_head        list;
 	struct blocking_notifier_head notifier;
+	struct rw_semaphore	sem;
 
 #ifdef CONFIG_PINCTRL
 	/*
-- 
2.37.2

Re: [PATCH v5 1/2] gpiolib: cdev: fix NULL-pointer dereferences

[Kent Gibson]

On Thu, Dec 01, 2022 at 09:33:34AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> There are several places where we can crash the kernel by requesting
> lines, unbinding the GPIO device, then calling any of the system calls
> relevant to the GPIO character device's annonymous file descriptors:
> ioctl(), read(), poll().
> 
> While I observed it with the GPIO simulator, it will also happen for any
> of the GPIO devices that can be hot-unplugged - for instance any HID GPIO
> expander (e.g. CP2112).
> 
> This affects both v1 and v2 uAPI.
> 
> This fixes it partially by checking if gdev->chip is not NULL but it
> doesn't entirely remedy the situation as we still have a race condition
> in which another thread can remove the device after the check.
> 
> Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
> Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
> Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> Fixes: 7b8e00d98168 ("gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

As per v4, I'm fine with whole series, so

Reviewed-by: Kent Gibson <warthog618@gmail.com>

Cheers,
Kent.

Re: [PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space

[Andy Shevchenko]

On Thu, Dec 01, 2022 at 09:33:35AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> While any of the GPIO cdev syscalls is in progress, the kernel can call
> gpiochip_remove() (for instance, when a USB GPIO expander is disconnected)
> which will set gdev->chip to NULL after which any subsequent access will
> cause a crash.
> 
> To avoid that: use an RW-semaphore in which the syscalls take it for
> reading (so that we don't needlessly prohibit the user-space from calling
> syscalls simultaneously) while gpiochip_remove() takes it for writing so
> that it can only happen once all syscalls return.
> 
> Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
> Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
> Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> Fixes: 7b8e00d98168 ("gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Kent Gibson <warthog618@gmail.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 166 +++++++++++++++++++++++++++++++-----
>  drivers/gpio/gpiolib.c      |   4 +
>  drivers/gpio/gpiolib.h      |   5 ++
>  3 files changed, 153 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 911d91668903..18c5e70ee7de 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -84,6 +84,53 @@ struct linehandle_state {
>  	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
>  	GPIOHANDLE_REQUEST_OPEN_SOURCE)
>  
> +typedef __poll_t (*poll_fn)(struct file *, struct poll_table_struct *);
> +typedef long (*ioctl_fn)(struct file *, unsigned int, unsigned long);
> +typedef ssize_t (*read_fn)(struct file *, char __user *,
> +			   size_t count, loff_t *);
> +
> +static __poll_t call_poll_locked(struct file *file,
> +				 struct poll_table_struct *wait,
> +				 struct gpio_device *gdev, poll_fn func)
> +{
> +	__poll_t ret;
> +
> +	if (!down_read_trylock(&gdev->sem))

> +		return 0;

EPOLLHUP?

> +	ret = func(file, wait);
> +	up_read(&gdev->sem);
> +
> +	return ret;
> +}
> +
> +static long call_ioctl_locked(struct file *file, unsigned int cmd,
> +			      unsigned long arg, struct gpio_device *gdev,
> +			      ioctl_fn func)
> +{
> +	long ret;
> +
> +	if (!down_read_trylock(&gdev->sem))
> +		return -ENODEV;
> +	ret = func(file, cmd, arg);
> +	up_read(&gdev->sem);
> +
> +	return ret;
> +}
> +
> +static ssize_t call_read_locked(struct file *file, char __user *buf,
> +				size_t count, loff_t *f_ps,
> +				struct gpio_device *gdev, read_fn func)
> +{
> +	ssize_t ret;
> +
> +	if (!down_read_trylock(&gdev->sem))
> +		return -ENODEV;
> +	ret = func(file, buf, count, f_ps);
> +	up_read(&gdev->sem);
> +
> +	return ret;
> +}
> +
>  static int linehandle_validate_flags(u32 flags)
>  {
>  	/* Return an error if an unknown flag is set */
> @@ -191,8 +238,8 @@ static long linehandle_set_config(struct linehandle_state *lh,
>  	return 0;
>  }
>  
> -static long linehandle_ioctl(struct file *file, unsigned int cmd,
> -			     unsigned long arg)
> +static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
> +				      unsigned long arg)
>  {
>  	struct linehandle_state *lh = file->private_data;
>  	void __user *ip = (void __user *)arg;
> @@ -250,6 +297,15 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
>  	}
>  }
>  
> +static long linehandle_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	struct linehandle_state *lh = file->private_data;
> +
> +	return call_ioctl_locked(file, cmd, arg, lh->gdev,
> +				 linehandle_ioctl_unlocked);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  static long linehandle_ioctl_compat(struct file *file, unsigned int cmd,
>  				    unsigned long arg)
> @@ -1381,8 +1437,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>  	return ret;
>  }
>  
> -static long linereq_ioctl(struct file *file, unsigned int cmd,
> -			  unsigned long arg)
> +static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
> +				   unsigned long arg)
>  {
>  	struct linereq *lr = file->private_data;
>  	void __user *ip = (void __user *)arg;
> @@ -1402,6 +1458,15 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
>  	}
>  }
>  
> +static long linereq_ioctl(struct file *file, unsigned int cmd,
> +			  unsigned long arg)
> +{
> +	struct linereq *lr = file->private_data;
> +
> +	return call_ioctl_locked(file, cmd, arg, lr->gdev,
> +				 linereq_ioctl_unlocked);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
>  				 unsigned long arg)
> @@ -1410,8 +1475,8 @@ static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
>  }
>  #endif
>  
> -static __poll_t linereq_poll(struct file *file,
> -			    struct poll_table_struct *wait)
> +static __poll_t linereq_poll_unlocked(struct file *file,
> +				      struct poll_table_struct *wait)
>  {
>  	struct linereq *lr = file->private_data;
>  	__poll_t events = 0;
> @@ -1428,10 +1493,16 @@ static __poll_t linereq_poll(struct file *file,
>  	return events;
>  }
>  
> -static ssize_t linereq_read(struct file *file,
> -			    char __user *buf,
> -			    size_t count,
> -			    loff_t *f_ps)
> +static __poll_t linereq_poll(struct file *file,
> +			     struct poll_table_struct *wait)
> +{
> +	struct linereq *lr = file->private_data;
> +
> +	return call_poll_locked(file, wait, lr->gdev, linereq_poll_unlocked);
> +}
> +
> +static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
> +				     size_t count, loff_t *f_ps)
>  {
>  	struct linereq *lr = file->private_data;
>  	struct gpio_v2_line_event le;
> @@ -1485,6 +1556,15 @@ static ssize_t linereq_read(struct file *file,
>  	return bytes_read;
>  }
>  
> +static ssize_t linereq_read(struct file *file, char __user *buf,
> +			    size_t count, loff_t *f_ps)
> +{
> +	struct linereq *lr = file->private_data;
> +
> +	return call_read_locked(file, buf, count, f_ps, lr->gdev,
> +				linereq_read_unlocked);
> +}
> +
>  static void linereq_free(struct linereq *lr)
>  {
>  	unsigned int i;
> @@ -1722,8 +1802,8 @@ struct lineevent_state {
>  	(GPIOEVENT_REQUEST_RISING_EDGE | \
>  	GPIOEVENT_REQUEST_FALLING_EDGE)
>  
> -static __poll_t lineevent_poll(struct file *file,
> -			       struct poll_table_struct *wait)
> +static __poll_t lineevent_poll_unlocked(struct file *file,
> +					struct poll_table_struct *wait)
>  {
>  	struct lineevent_state *le = file->private_data;
>  	__poll_t events = 0;
> @@ -1739,15 +1819,21 @@ static __poll_t lineevent_poll(struct file *file,
>  	return events;
>  }
>  
> +static __poll_t lineevent_poll(struct file *file,
> +			       struct poll_table_struct *wait)
> +{
> +	struct lineevent_state *le = file->private_data;
> +
> +	return call_poll_locked(file, wait, le->gdev, lineevent_poll_unlocked);
> +}
> +
>  struct compat_gpioeevent_data {
>  	compat_u64	timestamp;
>  	u32		id;
>  };
>  
> -static ssize_t lineevent_read(struct file *file,
> -			      char __user *buf,
> -			      size_t count,
> -			      loff_t *f_ps)
> +static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
> +				       size_t count, loff_t *f_ps)
>  {
>  	struct lineevent_state *le = file->private_data;
>  	struct gpioevent_data ge;
> @@ -1815,6 +1901,15 @@ static ssize_t lineevent_read(struct file *file,
>  	return bytes_read;
>  }
>  
> +static ssize_t lineevent_read(struct file *file, char __user *buf,
> +			      size_t count, loff_t *f_ps)
> +{
> +	struct lineevent_state *le = file->private_data;
> +
> +	return call_read_locked(file, buf, count, f_ps, le->gdev,
> +				lineevent_read_unlocked);
> +}
> +
>  static void lineevent_free(struct lineevent_state *le)
>  {
>  	if (le->irq)
> @@ -1832,8 +1927,8 @@ static int lineevent_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -static long lineevent_ioctl(struct file *file, unsigned int cmd,
> -			    unsigned long arg)
> +static long lineevent_ioctl_unlocked(struct file *file, unsigned int cmd,
> +				     unsigned long arg)
>  {
>  	struct lineevent_state *le = file->private_data;
>  	void __user *ip = (void __user *)arg;
> @@ -1864,6 +1959,15 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
>  	return -EINVAL;
>  }
>  
> +static long lineevent_ioctl(struct file *file, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	struct lineevent_state *le = file->private_data;
> +
> +	return call_ioctl_locked(file, cmd, arg, le->gdev,
> +				 lineevent_ioctl_unlocked);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  static long lineevent_ioctl_compat(struct file *file, unsigned int cmd,
>  				   unsigned long arg)
> @@ -2422,8 +2526,8 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> -static __poll_t lineinfo_watch_poll(struct file *file,
> -				    struct poll_table_struct *pollt)
> +static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
> +					     struct poll_table_struct *pollt)
>  {
>  	struct gpio_chardev_data *cdev = file->private_data;
>  	__poll_t events = 0;
> @@ -2440,8 +2544,17 @@ static __poll_t lineinfo_watch_poll(struct file *file,
>  	return events;
>  }
>  
> -static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
> -				   size_t count, loff_t *off)
> +static __poll_t lineinfo_watch_poll(struct file *file,
> +				    struct poll_table_struct *pollt)
> +{
> +	struct gpio_chardev_data *cdev = file->private_data;
> +
> +	return call_poll_locked(file, pollt, cdev->gdev,
> +				lineinfo_watch_poll_unlocked);
> +}
> +
> +static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
> +					    size_t count, loff_t *off)
>  {
>  	struct gpio_chardev_data *cdev = file->private_data;
>  	struct gpio_v2_line_info_changed event;
> @@ -2519,6 +2632,15 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
>  	return bytes_read;
>  }
>  
> +static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
> +				   size_t count, loff_t *off)
> +{
> +	struct gpio_chardev_data *cdev = file->private_data;
> +
> +	return call_read_locked(file, buf, count, off, cdev->gdev,
> +				lineinfo_watch_read_unlocked);
> +}
> +
>  /**
>   * gpio_chrdev_open() - open the chardev for ioctl operations
>   * @inode: inode for this chardev
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4756ea08894f..e0e73bd756ca 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -731,6 +731,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  
>  	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->notifier);
> +	init_rwsem(&gdev->sem);
>  
>  #ifdef CONFIG_PINCTRL
>  	INIT_LIST_HEAD(&gdev->pin_ranges);
> @@ -865,6 +866,8 @@ void gpiochip_remove(struct gpio_chip *gc)
>  	unsigned long	flags;
>  	unsigned int	i;
>  
> +	down_write(&gdev->sem);
> +
>  	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
>  	gpiochip_sysfs_unregister(gdev);
>  	gpiochip_free_hogs(gc);
> @@ -899,6 +902,7 @@ void gpiochip_remove(struct gpio_chip *gc)
>  	 * gone.
>  	 */
>  	gcdev_unregister(gdev);
> +	up_write(&gdev->sem);
>  	put_device(&gdev->dev);
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_remove);
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index d900ecdbac46..9ad68a0adf4a 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -15,6 +15,7 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/cdev.h>
> +#include <linux/rwsem.h>
>  
>  #define GPIOCHIP_NAME	"gpiochip"
>  
> @@ -39,6 +40,9 @@
>   * @list: links gpio_device:s together for traversal
>   * @notifier: used to notify subscribers about lines being requested, released
>   *            or reconfigured
> + * @sem: protects the structure from a NULL-pointer dereference of @chip by
> + *       user-space operations when the device gets unregistered during
> + *       a hot-unplug event
>   * @pin_ranges: range of pins served by the GPIO driver
>   *
>   * This state container holds most of the runtime variable data
> @@ -60,6 +64,7 @@ struct gpio_device {
>  	void			*data;
>  	struct list_head        list;
>  	struct blocking_notifier_head notifier;
> +	struct rw_semaphore	sem;
>  
>  #ifdef CONFIG_PINCTRL
>  	/*
> -- 
> 2.37.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] gpiolib: cdev: fix NULL-pointer dereferences

[Andy Shevchenko]

On Thu, Dec 01, 2022 at 09:33:34AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> There are several places where we can crash the kernel by requesting
> lines, unbinding the GPIO device, then calling any of the system calls
> relevant to the GPIO character device's annonymous file descriptors:
> ioctl(), read(), poll().
> 
> While I observed it with the GPIO simulator, it will also happen for any
> of the GPIO devices that can be hot-unplugged - for instance any HID GPIO
> expander (e.g. CP2112).
> 
> This affects both v1 and v2 uAPI.
> 
> This fixes it partially by checking if gdev->chip is not NULL but it
> doesn't entirely remedy the situation as we still have a race condition
> in which another thread can remove the device after the check.
> 
> Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
> Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
> Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> Fixes: 7b8e00d98168 ("gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 0cb6b468f364..911d91668903 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -201,6 +201,9 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
>  	unsigned int i;
>  	int ret;
>  
> +	if (!lh->gdev->chip)
> +		return -ENODEV;
> +
>  	switch (cmd) {
>  	case GPIOHANDLE_GET_LINE_VALUES_IOCTL:
>  		/* NOTE: It's okay to read values of output lines */
> @@ -1384,6 +1387,9 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
>  	struct linereq *lr = file->private_data;
>  	void __user *ip = (void __user *)arg;
>  
> +	if (!lr->gdev->chip)
> +		return -ENODEV;
> +
>  	switch (cmd) {
>  	case GPIO_V2_LINE_GET_VALUES_IOCTL:
>  		return linereq_get_values(lr, ip);
> @@ -1410,6 +1416,9 @@ static __poll_t linereq_poll(struct file *file,
>  	struct linereq *lr = file->private_data;
>  	__poll_t events = 0;
>  
> +	if (!lr->gdev->chip)
> +		return 0;

EPOLLHUP ?

>  	poll_wait(file, &lr->wait, wait);
>  
>  	if (!kfifo_is_empty_spinlocked_noirqsave(&lr->events,
> @@ -1429,6 +1438,9 @@ static ssize_t linereq_read(struct file *file,
>  	ssize_t bytes_read = 0;
>  	int ret;
>  
> +	if (!lr->gdev->chip)
> +		return -ENODEV;
> +
>  	if (count < sizeof(le))
>  		return -EINVAL;
>  
> @@ -1716,6 +1728,9 @@ static __poll_t lineevent_poll(struct file *file,
>  	struct lineevent_state *le = file->private_data;
>  	__poll_t events = 0;
>  
> +	if (!le->gdev->chip)
> +		return 0;
> +
>  	poll_wait(file, &le->wait, wait);
>  
>  	if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock))
> @@ -1740,6 +1755,9 @@ static ssize_t lineevent_read(struct file *file,
>  	ssize_t ge_size;
>  	int ret;
>  
> +	if (!le->gdev->chip)
> +		return -ENODEV;
> +
>  	/*
>  	 * When compatible system call is being used the struct gpioevent_data,
>  	 * in case of at least ia32, has different size due to the alignment
> @@ -1821,6 +1839,9 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
>  	void __user *ip = (void __user *)arg;
>  	struct gpiohandle_data ghd;
>  
> +	if (!le->gdev->chip)
> +		return -ENODEV;
> +
>  	/*
>  	 * We can get the value for an event line but not set it,
>  	 * because it is input by definition.
> @@ -2407,6 +2428,9 @@ static __poll_t lineinfo_watch_poll(struct file *file,
>  	struct gpio_chardev_data *cdev = file->private_data;
>  	__poll_t events = 0;
>  
> +	if (!cdev->gdev->chip)
> +		return 0;
> +
>  	poll_wait(file, &cdev->wait, pollt);
>  
>  	if (!kfifo_is_empty_spinlocked_noirqsave(&cdev->events,
> @@ -2425,6 +2449,9 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
>  	int ret;
>  	size_t event_size;
>  
> +	if (!cdev->gdev->chip)
> +		return -ENODEV;
> +
>  #ifndef CONFIG_GPIO_CDEV_V1
>  	event_size = sizeof(struct gpio_v2_line_info_changed);
>  	if (count < event_size)
> -- 
> 2.37.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space

[Bartosz Golaszewski]

On Thu, Dec 1, 2022 at 1:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 01, 2022 at 09:33:35AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > While any of the GPIO cdev syscalls is in progress, the kernel can call
> > gpiochip_remove() (for instance, when a USB GPIO expander is disconnected)
> > which will set gdev->chip to NULL after which any subsequent access will
> > cause a crash.
> >
> > To avoid that: use an RW-semaphore in which the syscalls take it for
> > reading (so that we don't needlessly prohibit the user-space from calling
> > syscalls simultaneously) while gpiochip_remove() takes it for writing so
> > that it can only happen once all syscalls return.
> >
> > Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
> > Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
> > Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> > Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> > Fixes: 7b8e00d98168 ("gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Reviewed-by: Kent Gibson <warthog618@gmail.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 166 +++++++++++++++++++++++++++++++-----
> >  drivers/gpio/gpiolib.c      |   4 +
> >  drivers/gpio/gpiolib.h      |   5 ++
> >  3 files changed, 153 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index 911d91668903..18c5e70ee7de 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -84,6 +84,53 @@ struct linehandle_state {
> >       GPIOHANDLE_REQUEST_OPEN_DRAIN | \
> >       GPIOHANDLE_REQUEST_OPEN_SOURCE)
> >
> > +typedef __poll_t (*poll_fn)(struct file *, struct poll_table_struct *);
> > +typedef long (*ioctl_fn)(struct file *, unsigned int, unsigned long);
> > +typedef ssize_t (*read_fn)(struct file *, char __user *,
> > +                        size_t count, loff_t *);
> > +
> > +static __poll_t call_poll_locked(struct file *file,
> > +                              struct poll_table_struct *wait,
> > +                              struct gpio_device *gdev, poll_fn func)
> > +{
> > +     __poll_t ret;
> > +
> > +     if (!down_read_trylock(&gdev->sem))
>
> > +             return 0;
>
> EPOLLHUP?
>

Or even EPOLLERR | EPOLLHUP.

Sorry in advance for the noise but I really want to get those fixes in
this week, so I'll send a new iteration shortly.

Bart