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 | 163 +++++++++++++++++++++++++++++++-----
drivers/gpio/gpiolib.c | 4 +
drivers/gpio/gpiolib.h | 5 ++
3 files changed, 150 insertions(+), 22 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 911d91668903..98904832f4b4 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -84,6 +84,50 @@ 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;
+
+ down_read(&gdev->sem);
+ 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;
+
+ down_read(&gdev->sem);
+ 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;
+
+ down_read(&gdev->sem);
+ 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 +235,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 +294,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 +1434,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 +1455,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 +1472,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 +1490,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 +1553,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 +1799,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 +1816,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 +1898,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 +1924,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 +1956,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 +2523,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 +2541,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 +2629,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.2On Wed, Nov 30, 2022 at 10:05:56AM +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.
Bikeshedding below and one question.
(As per tag I'm fine with this version anyway)
...
> +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 *);
<bikeshedding>
It's only 84 is on a single line.
Dunno if it's better to have typedef followed by wrapper pairs rather than
all typedefs and wrappers grouped.
</bikeshedding>
> +static __poll_t call_poll_locked(struct file *file,
> + struct poll_table_struct *wait,
> + struct gpio_device *gdev, poll_fn func)
> +{
> + __poll_t ret;
> + down_read(&gdev->sem);
Thinking more about this, wouldn't be better to actually
ret = down_read_trylock(&gdev->sem);
if (ret)
return ret;
?
> + ret = func(file, wait);
> + up_read(&gdev->sem);
> +
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 30, 2022 at 1:02 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 30, 2022 at 10:05:56AM +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.
>
> Bikeshedding below and one question.
> (As per tag I'm fine with this version anyway)
>
> ...
>
> > +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 *);
>
> <bikeshedding>
> It's only 84 is on a single line.
>
> Dunno if it's better to have typedef followed by wrapper pairs rather than
> all typedefs and wrappers grouped.
> </bikeshedding>
>
> > +static __poll_t call_poll_locked(struct file *file,
> > + struct poll_table_struct *wait,
> > + struct gpio_device *gdev, poll_fn func)
> > +{
> > + __poll_t ret;
>
> > + down_read(&gdev->sem);
>
> Thinking more about this, wouldn't be better to actually
>
> ret = down_read_trylock(&gdev->sem);
> if (ret)
> return ret;
>
> ?
You mean as in: try to take the lock, but if we're already removing
the device (as the down_write() can only happen in gpiochip_remove()),
then die right away? Smart! Yeah, I'll do it this way.
For the rest: I like my version better honestly.
Bart
On Wed, Nov 30, 2022 at 01:05:30PM +0100, Bartosz Golaszewski wrote: > On Wed, Nov 30, 2022 at 1:02 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Nov 30, 2022 at 10:05:56AM +0100, Bartosz Golaszewski wrote: > > > + down_read(&gdev->sem); > > > > Thinking more about this, wouldn't be better to actually > > > > ret = down_read_trylock(&gdev->sem); > > if (ret) > > return ret; > > > > ? > > You mean as in: try to take the lock, but if we're already removing > the device (as the down_write() can only happen in gpiochip_remove()), > then die right away? Smart! Yeah, I'll do it this way. But please check return values properly (it seems not obvious what it does return). -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.