Re: [PATCH] gpiolib: cdev: fix NULL-pointer dereferences

From: Kent Gibson
Date: Fri Nov 25 2022 - 11:24:58 EST


On Fri, Nov 25, 2022 at 04:32:57PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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.
>
> Fix this by simply checking if the GPIO chip pointer is not NULL.
>

Fixes: ??

And split for v1 and v2 as the Fixes for those will differ?

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
> drivers/gpio/gpiolib-cdev.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 0cb6b468f364..d5632742942a 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -201,6 +201,9 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
> unsigned int i;
> int ret;
>
> + if (!lh->gdev->chip)
> + return -ENODEV;
> +

Is there anything to prevent the chip being removed by another thread
between this check and subsequent usage?

Cheers,
Kent.

> switch (cmd) {
> case GPIOHANDLE_GET_LINE_VALUES_IOCTL:
> /* NOTE: It's okay to read values of output lines */
> @@ -1384,6 +1387,9 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
> struct linereq *lr = file->private_data;
> void __user *ip = (void __user *)arg;
>
> + if (!lr->gdev->chip)
> + return -ENODEV;
> +
> switch (cmd) {
> case GPIO_V2_LINE_GET_VALUES_IOCTL:
> return linereq_get_values(lr, ip);
> @@ -1716,6 +1722,9 @@ static __poll_t lineevent_poll(struct file *file,
> struct lineevent_state *le = file->private_data;
> __poll_t events = 0;
>
> + if (!le->gdev->chip)
> + return -ENODEV;
> +
> poll_wait(file, &le->wait, wait);
>
> if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock))
> @@ -1740,6 +1749,9 @@ static ssize_t lineevent_read(struct file *file,
> ssize_t ge_size;
> int ret;
>
> + if (!le->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
> @@ -1821,6 +1833,9 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
> void __user *ip = (void __user *)arg;
> struct gpiohandle_data ghd;
>
> + if (!le->gdev->chip)
> + return -ENODEV;
> +
> /*
> * We can get the value for an event line but not set it,
> * because it is input by definition.
> --
> 2.37.2
>