| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y4UI5w2Z+d1Df1KX@smile.fi.intel.com>
Date: Mon, 28 Nov 2022 21:15:51 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Kent Gibson <warthog618@...il.com>,
Linus Walleij <linus.walleij@...aro.org>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 1/2] gpiolib: cdev: fix NULL-pointer dereferences
On Mon, Nov 28, 2022 at 06:52:13PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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@...ux.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@...aro.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
>
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists