Message ID | 20221128175214.602612-3-brgl@bgdev.pl |
---|---|
State | New |
Headers | show |
Series | gpiolib: don't allow user-space to crash the kernel with hot-unplugs | expand |
On Mon, Nov 28, 2022 at 06:52:14PM +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. Missed also Dependency (the previous change). > 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") ... > + down_read(&gdev->sem); > > - if (!gdev->chip) > + if (!gdev->chip) { > + up_read(&gdev->sem); > return -ENODEV; > + } Wouldn't be easier to wrap existing functions (with their renaming) into a new ones with semaphore? You can even start that in the previous patch.
On Mon, Nov 28, 2022 at 8:21 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Nov 28, 2022 at 06:52:14PM +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. > > Missed also Dependency (the previous change). > > > 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") > > ... > > > + down_read(&gdev->sem); > > > > - if (!gdev->chip) > > + if (!gdev->chip) { > > + up_read(&gdev->sem); > > return -ENODEV; > > + } > > Wouldn't be easier to wrap existing functions (with their renaming) into a new > ones with semaphore? > You're probably right, it would be much cleaner. I'll see about it. Bart
Hi Bartosz, I love your patch! Yet something to improve: [auto build test ERROR on brgl/gpio/for-next] [also build test ERROR on linus/master v6.1-rc7 next-20221128] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpiolib-don-t-allow-user-space-to-crash-the-kernel-with-hot-unplugs/20221129-021037 base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next patch link: https://lore.kernel.org/r/20221128175214.602612-3-brgl%40bgdev.pl patch subject: [PATCH v2 2/2] gpiolib: protect the GPIO device against being dropped while in use by user-space config: arm-randconfig-r046-20221128 compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/ccf2821b643c23a5ef6fac8a4785c2127d4125c7 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Bartosz-Golaszewski/gpiolib-don-t-allow-user-space-to-crash-the-kernel-with-hot-unplugs/20221129-021037 git checkout ccf2821b643c23a5ef6fac8a4785c2127d4125c7 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpio/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All error/warnings (new ones prefixed by >>): drivers/gpio/gpiolib-cdev.c: In function 'lineinfo_watch_read': >> drivers/gpio/gpiolib-cdev.c:2647:1: error: expected 'while' before 'static' 2647 | static int gpio_chrdev_open(struct inode *inode, struct file *file) | ^~~~~~ >> drivers/gpio/gpiolib-cdev.c:2709:12: error: invalid storage class for function 'gpio_chrdev_release' 2709 | static int gpio_chrdev_release(struct inode *inode, struct file *file) | ^~~~~~~~~~~~~~~~~~~ >> drivers/gpio/gpiolib-cdev.c:2709:1: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] 2709 | static int gpio_chrdev_release(struct inode *inode, struct file *file) | ^~~~~~ >> drivers/gpio/gpiolib-cdev.c:2724:20: error: initializer element is not constant 2724 | .release = gpio_chrdev_release, | ^~~~~~~~~~~~~~~~~~~ drivers/gpio/gpiolib-cdev.c:2724:20: note: (near initialization for 'gpio_fileops.release') >> drivers/gpio/gpiolib-cdev.c:2725:17: error: 'gpio_chrdev_open' undeclared (first use in this function); did you mean 'gpio_chardev_data'? 2725 | .open = gpio_chrdev_open, | ^~~~~~~~~~~~~~~~ | gpio_chardev_data drivers/gpio/gpiolib-cdev.c:2725:17: note: each undeclared identifier is reported only once for each function it appears in >> drivers/gpio/gpiolib-cdev.c:2757:1: error: expected declaration or statement at end of input 2757 | } | ^ drivers/gpio/gpiolib-cdev.c: At top level: drivers/gpio/gpiolib-cdev.c:2754:6: warning: 'gpiolib_cdev_unregister' defined but not used [-Wunused-function] 2754 | void gpiolib_cdev_unregister(struct gpio_device *gdev) | ^~~~~~~~~~~~~~~~~~~~~~~ drivers/gpio/gpiolib-cdev.c:2736:5: warning: 'gpiolib_cdev_register' defined but not used [-Wunused-function] 2736 | int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt) | ^~~~~~~~~~~~~~~~~~~~~ drivers/gpio/gpiolib-cdev.c:2543:16: warning: 'lineinfo_watch_read' defined but not used [-Wunused-function] 2543 | static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, | ^~~~~~~~~~~~~~~~~~~ drivers/gpio/gpiolib-cdev.c:2494:12: warning: 'lineinfo_changed_notify' defined but not used [-Wunused-function] 2494 | static int lineinfo_changed_notify(struct notifier_block *nb, | ^~~~~~~~~~~~~~~~~~~~~~~ vim +2647 drivers/gpio/gpiolib-cdev.c 925ca36913fc7df Kent Gibson 2020-06-16 2640 925ca36913fc7df Kent Gibson 2020-06-16 2641 /** 925ca36913fc7df Kent Gibson 2020-06-16 2642 * gpio_chrdev_open() - open the chardev for ioctl operations 925ca36913fc7df Kent Gibson 2020-06-16 2643 * @inode: inode for this chardev 49bc52798d7bb66 Kent Gibson 2020-07-08 2644 * @file: file struct for storing private data 925ca36913fc7df Kent Gibson 2020-06-16 2645 * Returns 0 on success 925ca36913fc7df Kent Gibson 2020-06-16 2646 */ 49bc52798d7bb66 Kent Gibson 2020-07-08 @2647 static int gpio_chrdev_open(struct inode *inode, struct file *file) 925ca36913fc7df Kent Gibson 2020-06-16 2648 { 925ca36913fc7df Kent Gibson 2020-06-16 2649 struct gpio_device *gdev = container_of(inode->i_cdev, 925ca36913fc7df Kent Gibson 2020-06-16 2650 struct gpio_device, chrdev); e2b781c5f0dd45f Kent Gibson 2020-07-08 2651 struct gpio_chardev_data *cdev; 925ca36913fc7df Kent Gibson 2020-06-16 2652 int ret = -ENOMEM; 925ca36913fc7df Kent Gibson 2020-06-16 2653 ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2654 down_read(&gdev->sem); ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2655 925ca36913fc7df Kent Gibson 2020-06-16 2656 /* Fail on open if the backing gpiochip is gone */ ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2657 if (!gdev->chip) { ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2658 up_read(&gdev->sem); 925ca36913fc7df Kent Gibson 2020-06-16 2659 return -ENODEV; ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2660 } 925ca36913fc7df Kent Gibson 2020-06-16 2661 e2b781c5f0dd45f Kent Gibson 2020-07-08 2662 cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2663 if (!cdev) { ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2664 up_read(&gdev->sem); 925ca36913fc7df Kent Gibson 2020-06-16 2665 return -ENOMEM; ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2666 } 925ca36913fc7df Kent Gibson 2020-06-16 2667 e2b781c5f0dd45f Kent Gibson 2020-07-08 2668 cdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL); e2b781c5f0dd45f Kent Gibson 2020-07-08 2669 if (!cdev->watched_lines) e2b781c5f0dd45f Kent Gibson 2020-07-08 2670 goto out_free_cdev; 925ca36913fc7df Kent Gibson 2020-06-16 2671 e2b781c5f0dd45f Kent Gibson 2020-07-08 2672 init_waitqueue_head(&cdev->wait); e2b781c5f0dd45f Kent Gibson 2020-07-08 2673 INIT_KFIFO(cdev->events); e2b781c5f0dd45f Kent Gibson 2020-07-08 2674 cdev->gdev = gdev; 925ca36913fc7df Kent Gibson 2020-06-16 2675 e2b781c5f0dd45f Kent Gibson 2020-07-08 2676 cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify; 6accc376a7482ec Kent Gibson 2020-07-08 2677 ret = blocking_notifier_chain_register(&gdev->notifier, e2b781c5f0dd45f Kent Gibson 2020-07-08 2678 &cdev->lineinfo_changed_nb); 925ca36913fc7df Kent Gibson 2020-06-16 2679 if (ret) 925ca36913fc7df Kent Gibson 2020-06-16 2680 goto out_free_bitmap; 925ca36913fc7df Kent Gibson 2020-06-16 2681 925ca36913fc7df Kent Gibson 2020-06-16 2682 get_device(&gdev->dev); e2b781c5f0dd45f Kent Gibson 2020-07-08 2683 file->private_data = cdev; 925ca36913fc7df Kent Gibson 2020-06-16 2684 49bc52798d7bb66 Kent Gibson 2020-07-08 2685 ret = nonseekable_open(inode, file); 925ca36913fc7df Kent Gibson 2020-06-16 2686 if (ret) 925ca36913fc7df Kent Gibson 2020-06-16 2687 goto out_unregister_notifier; 925ca36913fc7df Kent Gibson 2020-06-16 2688 ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2689 up_read(&gdev->sem); 925ca36913fc7df Kent Gibson 2020-06-16 2690 return ret; 925ca36913fc7df Kent Gibson 2020-06-16 2691 925ca36913fc7df Kent Gibson 2020-06-16 2692 out_unregister_notifier: 6accc376a7482ec Kent Gibson 2020-07-08 2693 blocking_notifier_chain_unregister(&gdev->notifier, e2b781c5f0dd45f Kent Gibson 2020-07-08 2694 &cdev->lineinfo_changed_nb); 925ca36913fc7df Kent Gibson 2020-06-16 2695 out_free_bitmap: e2b781c5f0dd45f Kent Gibson 2020-07-08 2696 bitmap_free(cdev->watched_lines); e2b781c5f0dd45f Kent Gibson 2020-07-08 2697 out_free_cdev: e2b781c5f0dd45f Kent Gibson 2020-07-08 2698 kfree(cdev); ccf2821b643c23a Bartosz Golaszewski 2022-11-28 2699 up_read(&gdev->sem); 925ca36913fc7df Kent Gibson 2020-06-16 2700 return ret; 925ca36913fc7df Kent Gibson 2020-06-16 2701 } 925ca36913fc7df Kent Gibson 2020-06-16 2702 925ca36913fc7df Kent Gibson 2020-06-16 2703 /** 925ca36913fc7df Kent Gibson 2020-06-16 2704 * gpio_chrdev_release() - close chardev after ioctl operations 925ca36913fc7df Kent Gibson 2020-06-16 2705 * @inode: inode for this chardev 49bc52798d7bb66 Kent Gibson 2020-07-08 2706 * @file: file struct for storing private data 925ca36913fc7df Kent Gibson 2020-06-16 2707 * Returns 0 on success 925ca36913fc7df Kent Gibson 2020-06-16 2708 */ 49bc52798d7bb66 Kent Gibson 2020-07-08 @2709 static int gpio_chrdev_release(struct inode *inode, struct file *file) 925ca36913fc7df Kent Gibson 2020-06-16 2710 { e2b781c5f0dd45f Kent Gibson 2020-07-08 2711 struct gpio_chardev_data *cdev = file->private_data; e2b781c5f0dd45f Kent Gibson 2020-07-08 2712 struct gpio_device *gdev = cdev->gdev; 925ca36913fc7df Kent Gibson 2020-06-16 2713 e2b781c5f0dd45f Kent Gibson 2020-07-08 2714 bitmap_free(cdev->watched_lines); 6accc376a7482ec Kent Gibson 2020-07-08 2715 blocking_notifier_chain_unregister(&gdev->notifier, e2b781c5f0dd45f Kent Gibson 2020-07-08 2716 &cdev->lineinfo_changed_nb); 925ca36913fc7df Kent Gibson 2020-06-16 2717 put_device(&gdev->dev); e2b781c5f0dd45f Kent Gibson 2020-07-08 2718 kfree(cdev); 925ca36913fc7df Kent Gibson 2020-06-16 2719 925ca36913fc7df Kent Gibson 2020-06-16 2720 return 0; 925ca36913fc7df Kent Gibson 2020-06-16 2721 } 925ca36913fc7df Kent Gibson 2020-06-16 2722 925ca36913fc7df Kent Gibson 2020-06-16 2723 static const struct file_operations gpio_fileops = { 925ca36913fc7df Kent Gibson 2020-06-16 @2724 .release = gpio_chrdev_release, 925ca36913fc7df Kent Gibson 2020-06-16 @2725 .open = gpio_chrdev_open, 925ca36913fc7df Kent Gibson 2020-06-16 2726 .poll = lineinfo_watch_poll, 925ca36913fc7df Kent Gibson 2020-06-16 2727 .read = lineinfo_watch_read, 925ca36913fc7df Kent Gibson 2020-06-16 2728 .owner = THIS_MODULE, 925ca36913fc7df Kent Gibson 2020-06-16 2729 .llseek = no_llseek, 925ca36913fc7df Kent Gibson 2020-06-16 2730 .unlocked_ioctl = gpio_ioctl, 925ca36913fc7df Kent Gibson 2020-06-16 2731 #ifdef CONFIG_COMPAT 925ca36913fc7df Kent Gibson 2020-06-16 2732 .compat_ioctl = gpio_ioctl_compat, 925ca36913fc7df Kent Gibson 2020-06-16 2733 #endif 925ca36913fc7df Kent Gibson 2020-06-16 2734 }; 925ca36913fc7df Kent Gibson 2020-06-16 2735 925ca36913fc7df Kent Gibson 2020-06-16 2736 int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt) 925ca36913fc7df Kent Gibson 2020-06-16 2737 { 925ca36913fc7df Kent Gibson 2020-06-16 2738 int ret; 925ca36913fc7df Kent Gibson 2020-06-16 2739 925ca36913fc7df Kent Gibson 2020-06-16 2740 cdev_init(&gdev->chrdev, &gpio_fileops); 925ca36913fc7df Kent Gibson 2020-06-16 2741 gdev->chrdev.owner = THIS_MODULE; 925ca36913fc7df Kent Gibson 2020-06-16 2742 gdev->dev.devt = MKDEV(MAJOR(devt), gdev->id); 925ca36913fc7df Kent Gibson 2020-06-16 2743 925ca36913fc7df Kent Gibson 2020-06-16 2744 ret = cdev_device_add(&gdev->chrdev, &gdev->dev); 925ca36913fc7df Kent Gibson 2020-06-16 2745 if (ret) 925ca36913fc7df Kent Gibson 2020-06-16 2746 return ret; 925ca36913fc7df Kent Gibson 2020-06-16 2747 925ca36913fc7df Kent Gibson 2020-06-16 2748 chip_dbg(gdev->chip, "added GPIO chardev (%d:%d)\n", 925ca36913fc7df Kent Gibson 2020-06-16 2749 MAJOR(devt), gdev->id); 925ca36913fc7df Kent Gibson 2020-06-16 2750 925ca36913fc7df Kent Gibson 2020-06-16 2751 return 0; 925ca36913fc7df Kent Gibson 2020-06-16 2752 } 925ca36913fc7df Kent Gibson 2020-06-16 2753 925ca36913fc7df Kent Gibson 2020-06-16 2754 void gpiolib_cdev_unregister(struct gpio_device *gdev) 925ca36913fc7df Kent Gibson 2020-06-16 2755 { 925ca36913fc7df Kent Gibson 2020-06-16 2756 cdev_device_del(&gdev->chrdev, &gdev->dev); 925ca36913fc7df Kent Gibson 2020-06-16 @2757 }
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 7a9504fb27f1..581bc0e945a4 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -200,10 +200,14 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd, struct gpiohandle_data ghd; DECLARE_BITMAP(vals, GPIOHANDLES_MAX); unsigned int i; - int ret; + int ret = 0; + + down_read(&gdev->sem); - if (!gdev->chip) + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } switch (cmd) { case GPIOHANDLE_GET_LINE_VALUES_IOCTL: @@ -212,43 +216,52 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd, lh->num_descs, lh->descs, NULL, vals); if (ret) - return ret; + break; memset(&ghd, 0, sizeof(ghd)); for (i = 0; i < lh->num_descs; i++) ghd.values[i] = test_bit(i, vals); if (copy_to_user(ip, &ghd, sizeof(ghd))) - return -EFAULT; + ret = -EFAULT; - return 0; + break; case GPIOHANDLE_SET_LINE_VALUES_IOCTL: /* * All line descriptors were created at once with the same * flags so just check if the first one is really output. */ - if (!test_bit(FLAG_IS_OUT, &lh->descs[0]->flags)) - return -EPERM; + if (!test_bit(FLAG_IS_OUT, &lh->descs[0]->flags)) { + ret = -EPERM; + break; + } - if (copy_from_user(&ghd, ip, sizeof(ghd))) - return -EFAULT; + if (copy_from_user(&ghd, ip, sizeof(ghd))) { + ret = -EFAULT; + break; + } /* Clamp all values to [0,1] */ for (i = 0; i < lh->num_descs; i++) __assign_bit(i, vals, ghd.values[i]); /* Reuse the array setting function */ - return gpiod_set_array_value_complex(false, - true, - lh->num_descs, - lh->descs, - NULL, - vals); + ret = gpiod_set_array_value_complex(false, + true, + lh->num_descs, + lh->descs, + NULL, + vals); + break; case GPIOHANDLE_SET_CONFIG_IOCTL: - return linehandle_set_config(lh, ip); + ret = linehandle_set_config(lh, ip); + break; default: - return -EINVAL; + ret = -EINVAL; } + + up_read(&gdev->sem); + return ret; } #ifdef CONFIG_COMPAT @@ -1388,20 +1401,31 @@ static long linereq_ioctl(struct file *file, unsigned int cmd, struct linereq *lr = file->private_data; struct gpio_device *gdev = lr->gdev; void __user *ip = (void __user *)arg; + long ret; + + down_read(&gdev->sem); - if (!gdev->chip) + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } switch (cmd) { case GPIO_V2_LINE_GET_VALUES_IOCTL: - return linereq_get_values(lr, ip); + ret = linereq_get_values(lr, ip); + break; case GPIO_V2_LINE_SET_VALUES_IOCTL: - return linereq_set_values(lr, ip); + ret = linereq_set_values(lr, ip); + break; case GPIO_V2_LINE_SET_CONFIG_IOCTL: - return linereq_set_config(lr, ip); + ret = linereq_set_config(lr, ip); + break; default: - return -EINVAL; + ret = -EINVAL; } + + up_read(&gdev->sem); + return ret; } #ifdef CONFIG_COMPAT @@ -1419,8 +1443,12 @@ static __poll_t linereq_poll(struct file *file, struct gpio_device *gdev = lr->gdev; __poll_t events = 0; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return 0; + } poll_wait(file, &lr->wait, wait); @@ -1428,6 +1456,7 @@ static __poll_t linereq_poll(struct file *file, &lr->wait.lock)) events = EPOLLIN | EPOLLRDNORM; + up_read(&gdev->sem); return events; } @@ -1442,22 +1471,30 @@ static ssize_t linereq_read(struct file *file, ssize_t bytes_read = 0; int ret; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } - if (count < sizeof(le)) + if (count < sizeof(le)) { + up_read(&gdev->sem); return -EINVAL; + } do { spin_lock(&lr->wait.lock); if (kfifo_is_empty(&lr->events)) { if (bytes_read) { spin_unlock(&lr->wait.lock); + up_read(&gdev->sem); return bytes_read; } if (file->f_flags & O_NONBLOCK) { spin_unlock(&lr->wait.lock); + up_read(&gdev->sem); return -EAGAIN; } @@ -1465,6 +1502,7 @@ static ssize_t linereq_read(struct file *file, !kfifo_is_empty(&lr->events)); if (ret) { spin_unlock(&lr->wait.lock); + up_read(&gdev->sem); return ret; } } @@ -1481,11 +1519,14 @@ static ssize_t linereq_read(struct file *file, break; } - if (copy_to_user(buf + bytes_read, &le, sizeof(le))) + if (copy_to_user(buf + bytes_read, &le, sizeof(le))) { + up_read(&gdev->sem); return -EFAULT; + } bytes_read += sizeof(le); } while (count >= bytes_read + sizeof(le)); + up_read(&gdev->sem); return bytes_read; } @@ -1733,14 +1774,19 @@ static __poll_t lineevent_poll(struct file *file, struct gpio_device *gdev = le->gdev; __poll_t events = 0; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return 0; + } poll_wait(file, &le->wait, wait); if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock)) events = EPOLLIN | EPOLLRDNORM; + up_read(&gdev->sem); return events; } @@ -1761,8 +1807,12 @@ static ssize_t lineevent_read(struct file *file, ssize_t ge_size; int ret; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } /* * When compatible system call is being used the struct gpioevent_data, @@ -1777,19 +1827,23 @@ static ssize_t lineevent_read(struct file *file, ge_size = sizeof(struct compat_gpioeevent_data); else ge_size = sizeof(struct gpioevent_data); - if (count < ge_size) + if (count < ge_size) { + up_read(&gdev->sem); return -EINVAL; + } do { spin_lock(&le->wait.lock); if (kfifo_is_empty(&le->events)) { if (bytes_read) { spin_unlock(&le->wait.lock); + up_read(&gdev->sem); return bytes_read; } if (file->f_flags & O_NONBLOCK) { spin_unlock(&le->wait.lock); + up_read(&gdev->sem); return -EAGAIN; } @@ -1797,6 +1851,7 @@ static ssize_t lineevent_read(struct file *file, !kfifo_is_empty(&le->events)); if (ret) { spin_unlock(&le->wait.lock); + up_read(&gdev->sem); return ret; } } @@ -1813,11 +1868,14 @@ static ssize_t lineevent_read(struct file *file, break; } - if (copy_to_user(buf + bytes_read, &ge, ge_size)) + if (copy_to_user(buf + bytes_read, &ge, ge_size)) { + up_read(&gdev->sem); return -EFAULT; + } bytes_read += ge_size; } while (count >= bytes_read + ge_size); + up_read(&gdev->sem); return bytes_read; } @@ -1846,8 +1904,12 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd, void __user *ip = (void __user *)arg; struct gpiohandle_data ghd; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } /* * We can get the value for an event line but not set it, @@ -1859,15 +1921,23 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd, memset(&ghd, 0, sizeof(ghd)); val = gpiod_get_value_cansleep(le->desc); - if (val < 0) + if (val < 0) { + up_read(&gdev->sem); return val; + } + ghd.values[0] = val; - if (copy_to_user(ip, &ghd, sizeof(ghd))) + if (copy_to_user(ip, &ghd, sizeof(ghd))) { + up_read(&gdev->sem); return -EFAULT; + } + up_read(&gdev->sem); return 0; } + + up_read(&gdev->sem); return -EINVAL; } @@ -2358,36 +2428,53 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct gpio_chardev_data *cdev = file->private_data; struct gpio_device *gdev = cdev->gdev; void __user *ip = (void __user *)arg; + long ret; + + down_read(&gdev->sem); /* We fail any subsequent ioctl():s when the chip is gone */ - if (!gdev->chip) + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } /* Fill in the struct and pass to userspace */ switch (cmd) { case GPIO_GET_CHIPINFO_IOCTL: - return chipinfo_get(cdev, ip); + ret = chipinfo_get(cdev, ip); + break; #ifdef CONFIG_GPIO_CDEV_V1 case GPIO_GET_LINEHANDLE_IOCTL: - return linehandle_create(gdev, ip); + ret = linehandle_create(gdev, ip); + break; case GPIO_GET_LINEEVENT_IOCTL: - return lineevent_create(gdev, ip); + ret = lineevent_create(gdev, ip); + break; case GPIO_GET_LINEINFO_IOCTL: - return lineinfo_get_v1(cdev, ip, false); + ret = lineinfo_get_v1(cdev, ip, false); + break; case GPIO_GET_LINEINFO_WATCH_IOCTL: - return lineinfo_get_v1(cdev, ip, true); + ret = lineinfo_get_v1(cdev, ip, true); + break; #endif /* CONFIG_GPIO_CDEV_V1 */ case GPIO_V2_GET_LINEINFO_IOCTL: - return lineinfo_get(cdev, ip, false); + ret = lineinfo_get(cdev, ip, false); + break; case GPIO_V2_GET_LINEINFO_WATCH_IOCTL: - return lineinfo_get(cdev, ip, true); + ret = lineinfo_get(cdev, ip, true); + break; case GPIO_V2_GET_LINE_IOCTL: - return linereq_create(gdev, ip); + ret = linereq_create(gdev, ip); + break; case GPIO_GET_LINEINFO_UNWATCH_IOCTL: - return lineinfo_unwatch(cdev, ip); + ret = lineinfo_unwatch(cdev, ip); + break; default: - return -EINVAL; + ret = -EINVAL; } + + up_read(&gdev->sem); + return ret; } #ifdef CONFIG_COMPAT @@ -2436,8 +2523,12 @@ static __poll_t lineinfo_watch_poll(struct file *file, struct gpio_device *gdev = cdev->gdev; __poll_t events = 0; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return 0; + } poll_wait(file, &cdev->wait, pollt); @@ -2445,6 +2536,7 @@ static __poll_t lineinfo_watch_poll(struct file *file, &cdev->wait.lock)) events = EPOLLIN | EPOLLRDNORM; + up_read(&gdev->sem); return events; } @@ -2458,13 +2550,19 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, int ret; size_t event_size; - if (!gdev->chip) + down_read(&gdev->sem); + + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } #ifndef CONFIG_GPIO_CDEV_V1 event_size = sizeof(struct gpio_v2_line_info_changed); - if (count < event_size) + if (count < event_size) { + up_read(&gdev->sem); return -EINVAL; + } #endif do { @@ -2472,11 +2570,13 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, if (kfifo_is_empty(&cdev->events)) { if (bytes_read) { spin_unlock(&cdev->wait.lock); + up_read(&gdev->sem); return bytes_read; } if (file->f_flags & O_NONBLOCK) { spin_unlock(&cdev->wait.lock); + up_read(&gdev->sem); return -EAGAIN; } @@ -2484,6 +2584,7 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, !kfifo_is_empty(&cdev->events)); if (ret) { spin_unlock(&cdev->wait.lock); + up_read(&gdev->sem); return ret; } } @@ -2495,6 +2596,7 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, event_size = sizeof(struct gpioline_info_changed); if (count < event_size) { spin_unlock(&cdev->wait.lock); + up_read(&gdev->sem); return -EINVAL; } #endif @@ -2508,23 +2610,31 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, #ifdef CONFIG_GPIO_CDEV_V1 if (event_size == sizeof(struct gpio_v2_line_info_changed)) { - if (copy_to_user(buf + bytes_read, &event, event_size)) + if (copy_to_user(buf + bytes_read, + &event, event_size)) { + up_read(&gdev->sem); return -EFAULT; + } } else { struct gpioline_info_changed event_v1; gpio_v2_line_info_changed_to_v1(&event, &event_v1); if (copy_to_user(buf + bytes_read, &event_v1, - event_size)) + event_size)) { + up_read(&gdev->sem); return -EFAULT; + } } #else - if (copy_to_user(buf + bytes_read, &event, event_size)) + if (copy_to_user(buf + bytes_read, &event, event_size)) { + up_read(&gdev->sem); return -EFAULT; + #endif bytes_read += event_size; } while (count >= bytes_read + sizeof(event)); + up_read(&gdev->sem); return bytes_read; } @@ -2541,13 +2651,19 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) struct gpio_chardev_data *cdev; int ret = -ENOMEM; + down_read(&gdev->sem); + /* Fail on open if the backing gpiochip is gone */ - if (!gdev->chip) + if (!gdev->chip) { + up_read(&gdev->sem); return -ENODEV; + } cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); - if (!cdev) + if (!cdev) { + up_read(&gdev->sem); return -ENOMEM; + } cdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL); if (!cdev->watched_lines) @@ -2570,6 +2686,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) if (ret) goto out_unregister_notifier; + up_read(&gdev->sem); return ret; out_unregister_notifier: @@ -2579,6 +2696,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) bitmap_free(cdev->watched_lines); out_free_cdev: kfree(cdev); + up_read(&gdev->sem); return ret; } diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4756ea08894f..0d71f8a5a66e 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,7 @@ 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 +901,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 /*