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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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