| Message ID | 20221128175214.602612-2-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:13PM +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. Fine by me, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Not that I'm insisting, but see a nit-pick below. I think it's better for the sake of maintenance in a long term. > 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> > --- > drivers/gpio/gpiolib-cdev.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 0cb6b468f364..7a9504fb27f1 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -195,12 +195,16 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > struct linehandle_state *lh = file->private_data; > + struct gpio_device *gdev = lh->gdev; > void __user *ip = (void __user *)arg; > struct gpiohandle_data ghd; > DECLARE_BITMAP(vals, GPIOHANDLES_MAX); > unsigned int i; > int ret; I would split definition and assignment and put latter here as: gdev = lh->gdev; Same to other places. > + if (!gdev->chip) > + return -ENODEV; > + > switch (cmd) { > case GPIOHANDLE_GET_LINE_VALUES_IOCTL: > /* NOTE: It's okay to read values of output lines */ > @@ -1382,8 +1386,12 @@ static long linereq_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > struct linereq *lr = file->private_data; > + struct gpio_device *gdev = lr->gdev; > void __user *ip = (void __user *)arg; > > + if (!gdev->chip) > + return -ENODEV; > + > switch (cmd) { > case GPIO_V2_LINE_GET_VALUES_IOCTL: > return linereq_get_values(lr, ip); > @@ -1408,8 +1416,12 @@ static __poll_t linereq_poll(struct file *file, > struct poll_table_struct *wait) > { > struct linereq *lr = file->private_data; > + struct gpio_device *gdev = lr->gdev; > __poll_t events = 0; > > + if (!gdev->chip) > + return 0; > + > poll_wait(file, &lr->wait, wait); > > if (!kfifo_is_empty_spinlocked_noirqsave(&lr->events, > @@ -1425,10 +1437,14 @@ static ssize_t linereq_read(struct file *file, > loff_t *f_ps) > { > struct linereq *lr = file->private_data; > + struct gpio_device *gdev = lr->gdev; > struct gpio_v2_line_event le; > ssize_t bytes_read = 0; > int ret; > > + if (!gdev->chip) > + return -ENODEV; > + > if (count < sizeof(le)) > return -EINVAL; > > @@ -1714,8 +1730,12 @@ static __poll_t lineevent_poll(struct file *file, > struct poll_table_struct *wait) > { > struct lineevent_state *le = file->private_data; > + struct gpio_device *gdev = le->gdev; > __poll_t events = 0; > > + if (!gdev->chip) > + return 0; > + > poll_wait(file, &le->wait, wait); > > if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock)) > @@ -1735,11 +1755,15 @@ static ssize_t lineevent_read(struct file *file, > loff_t *f_ps) > { > struct lineevent_state *le = file->private_data; > + struct gpio_device *gdev = le->gdev; > struct gpioevent_data ge; > ssize_t bytes_read = 0; > ssize_t ge_size; > int ret; > > + if (!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 > @@ -1818,9 +1842,13 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > struct lineevent_state *le = file->private_data; > + struct gpio_device *gdev = le->gdev; > void __user *ip = (void __user *)arg; > struct gpiohandle_data ghd; > > + if (!gdev->chip) > + return -ENODEV; > + > /* > * We can get the value for an event line but not set it, > * because it is input by definition. > @@ -2405,8 +2433,12 @@ static __poll_t lineinfo_watch_poll(struct file *file, > struct poll_table_struct *pollt) > { > struct gpio_chardev_data *cdev = file->private_data; > + struct gpio_device *gdev = cdev->gdev; > __poll_t events = 0; > > + if (!gdev->chip) > + return 0; > + > poll_wait(file, &cdev->wait, pollt); > > if (!kfifo_is_empty_spinlocked_noirqsave(&cdev->events, > @@ -2420,11 +2452,15 @@ 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; > + struct gpio_device *gdev = cdev->gdev; > struct gpio_v2_line_info_changed event; > ssize_t bytes_read = 0; > int ret; > size_t event_size; > > + if (!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 >
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 0cb6b468f364..7a9504fb27f1 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -195,12 +195,16 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct linehandle_state *lh = file->private_data; + struct gpio_device *gdev = lh->gdev; void __user *ip = (void __user *)arg; struct gpiohandle_data ghd; DECLARE_BITMAP(vals, GPIOHANDLES_MAX); unsigned int i; int ret; + if (!gdev->chip) + return -ENODEV; + switch (cmd) { case GPIOHANDLE_GET_LINE_VALUES_IOCTL: /* NOTE: It's okay to read values of output lines */ @@ -1382,8 +1386,12 @@ static long linereq_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct linereq *lr = file->private_data; + struct gpio_device *gdev = lr->gdev; void __user *ip = (void __user *)arg; + if (!gdev->chip) + return -ENODEV; + switch (cmd) { case GPIO_V2_LINE_GET_VALUES_IOCTL: return linereq_get_values(lr, ip); @@ -1408,8 +1416,12 @@ static __poll_t linereq_poll(struct file *file, struct poll_table_struct *wait) { struct linereq *lr = file->private_data; + struct gpio_device *gdev = lr->gdev; __poll_t events = 0; + if (!gdev->chip) + return 0; + poll_wait(file, &lr->wait, wait); if (!kfifo_is_empty_spinlocked_noirqsave(&lr->events, @@ -1425,10 +1437,14 @@ static ssize_t linereq_read(struct file *file, loff_t *f_ps) { struct linereq *lr = file->private_data; + struct gpio_device *gdev = lr->gdev; struct gpio_v2_line_event le; ssize_t bytes_read = 0; int ret; + if (!gdev->chip) + return -ENODEV; + if (count < sizeof(le)) return -EINVAL; @@ -1714,8 +1730,12 @@ static __poll_t lineevent_poll(struct file *file, struct poll_table_struct *wait) { struct lineevent_state *le = file->private_data; + struct gpio_device *gdev = le->gdev; __poll_t events = 0; + if (!gdev->chip) + return 0; + poll_wait(file, &le->wait, wait); if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock)) @@ -1735,11 +1755,15 @@ static ssize_t lineevent_read(struct file *file, loff_t *f_ps) { struct lineevent_state *le = file->private_data; + struct gpio_device *gdev = le->gdev; struct gpioevent_data ge; ssize_t bytes_read = 0; ssize_t ge_size; int ret; + if (!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 @@ -1818,9 +1842,13 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct lineevent_state *le = file->private_data; + struct gpio_device *gdev = le->gdev; void __user *ip = (void __user *)arg; struct gpiohandle_data ghd; + if (!gdev->chip) + return -ENODEV; + /* * We can get the value for an event line but not set it, * because it is input by definition. @@ -2405,8 +2433,12 @@ static __poll_t lineinfo_watch_poll(struct file *file, struct poll_table_struct *pollt) { struct gpio_chardev_data *cdev = file->private_data; + struct gpio_device *gdev = cdev->gdev; __poll_t events = 0; + if (!gdev->chip) + return 0; + poll_wait(file, &cdev->wait, pollt); if (!kfifo_is_empty_spinlocked_noirqsave(&cdev->events, @@ -2420,11 +2452,15 @@ 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; + struct gpio_device *gdev = cdev->gdev; struct gpio_v2_line_info_changed event; ssize_t bytes_read = 0; int ret; size_t event_size; + if (!gdev->chip) + return -ENODEV; + #ifndef CONFIG_GPIO_CDEV_V1 event_size = sizeof(struct gpio_v2_line_info_changed); if (count < event_size)