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

From: Andy Shevchenko
Date: Thu Dec 01 2022 - 07:01:46 EST

Next message: Yang Yingliang: "Re: [PATCH v2] chardev: fix error handling in cdev_device_add()"
Previous message: Liu Shixin: "[PATCH] jfs: fix invalid free of JFS_IP(ipimap)->i_imap in diUnmount"
In reply to: Bartosz Golaszewski: "[PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space"
Next in thread: Bartosz Golaszewski: "Re: [PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Dec 01, 2022 at 09:33:35AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Kent Gibson <warthog618@xxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> 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

Next message: Yang Yingliang: "Re: [PATCH v2] chardev: fix error handling in cdev_device_add()"
Previous message: Liu Shixin: "[PATCH] jfs: fix invalid free of JFS_IP(ipimap)->i_imap in diUnmount"
In reply to: Bartosz Golaszewski: "[PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space"
Next in thread: Bartosz Golaszewski: "Re: [PATCH v5 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]