linux-kernel - Re: [PATCH] gpio: Restrict usage of gc irq members before initialization

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bkydc02s.fsf@collabora.com>
Date:   Thu, 10 Mar 2022 10:13:31 -0500
From:   Gabriel Krisman Bertazi <krisman@...labora.com>
To:     Shreeya Patel <shreeya.patel@...labora.com>
Cc:     linus.walleij@...aro.org, brgl@...ev.pl,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com, andy.shevchenko@...il.com
Subject: Re: [PATCH] gpio: Restrict usage of gc irq members before
 initialization

Shreeya Patel <shreeya.patel@...labora.com> writes:

> gc irq members are exposed before they could be completely
> initialized and this leads to race conditions.
>
> One such issue was observed for the gc->irq.domain variable which
> was accessed through the I2C interface in gpiochip_to_irq() before
> it could be initialized by gpiochip_add_irqchip(). This resulted in
> Kernel NULL pointer dereference.
>
> To avoid such scenarios, restrict usage of gc irq members before
> they are completely initialized.
>
> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
> ---
>
> Following is the NULL pointer dereference Oops for reference :-
>
> kernel: Call Trace:
> kernel:  gpiod_to_irq+0x53/0x70
> kernel:  acpi_dev_gpio_irq_get_by+0x113/0x1f0
> kernel:  i2c_acpi_get_irq+0xc0/0xd0
> kernel:  i2c_device_probe+0x28a/0x2a0
> kernel:  really_probe+0xf2/0x460
> kernel:  driver_probe_device+0xe8/0x160
> kernel:  ? driver_allows_async_probing+0x50/0x50
> kernel:  bus_for_each_drv+0x8f/0xd0
> kernel:  __device_attach_async_helper+0x9f/0xf0
> kernel:  async_run_entry_fn+0x2e/0x110
> kernel:  process_one_work+0x214/0x3e0
> kernel:  worker_thread+0x4d/0x3d0
> kernel:  ? process_one_work+0x3e0/0x3e0
> kernel:  kthread+0x133/0x150
> kernel:  ? kthread_associate_blkcg+0xc0/0xc0
> kernel:  ret_from_fork+0x22/0x30
> kernel: CR2: 0000000000000028
> kernel: ---[ end trace d0f5a7a0e0eb268f ]---
> kernel: RIP: 0010:gpiochip_to_irq+0x47/0xc0
>
>  drivers/gpio/gpiolib.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index defb7c464b87..2c6f382ff159 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -90,6 +90,7 @@ static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gc);
>  static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc);
>  
>  static bool gpiolib_initialized;
> +bool gc_irq_initialized;

What if you have more than one gc?  this needs to be an attribute of the
gc, not global.

>  
>  static inline void desc_set_label(struct gpio_desc *d, const char *label)
>  {
> @@ -1593,6 +1594,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>  
>  	acpi_gpiochip_request_interrupts(gc);
>  
> +	gc_irq_initialized = true;
> +

this assignment can be reordered by the compiler, in which
case (gc->domain == NULL && gc_irq_initialized == true).

>  	return 0;
>  }
>  
> @@ -3138,7 +3141,7 @@ int gpiod_to_irq(const struct gpio_desc *desc)
>  
>  	gc = desc->gdev->chip;
>  	offset = gpio_chip_hwgpio(desc);
> -	if (gc->to_irq) {
> +	if (gc->to_irq && gc_irq_initialized) {
>  		int retirq = gc->to_irq(gc, offset);
>  
>  		/* Zero means NO_IRQ */

-- 
Gabriel Krisman Bertazi