[PATCH] gpio: Restrict usage of gc irq members before initialization

[PATCH] gpio: Restrict usage of gc irq members before initialization

[Shreeya Patel]

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@collabora.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;
 
 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;
+
 	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 */
-- 
2.30.2

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

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 3:22 PM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> 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.

Fixes tag?

...

> +bool gc_irq_initialized;

Non-static?

Why is it global?

...

> @@ -1593,6 +1594,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>
>         acpi_gpiochip_request_interrupts(gc);
>
> +       gc_irq_initialized = true;

This is wrong. Imagine a system where you have more than one GPIO chip.

...

> -       if (gc->to_irq) {
> +       if (gc->to_irq && gc_irq_initialized) {
>                 int retirq = gc->to_irq(gc, offset);

Shouldn't it rather be something like

  if (gc->to_irq) {
    if (! ..._initialized)
      return -EPROBE_DEFER;
    ...
  }

?

--
With Best Regards,
Andy Shevchenko

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

[Shreeya Patel]

On 10/03/22 8:14 pm, Andy Shevchenko wrote:
> On Thu, Mar 10, 2022 at 3:22 PM Shreeya Patel
> <shreeya.patel@collabora.com> wrote:
>> 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.
> Fixes tag?
>
> ...


We don't really have any specific commit which introduces this issue so
I did not add fixes tag here.

>> +bool gc_irq_initialized;
> Non-static?
>
> Why is it global?
>
> ...
>
>> @@ -1593,6 +1594,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>>
>>          acpi_gpiochip_request_interrupts(gc);
>>
>> +       gc_irq_initialized = true;
> This is wrong. Imagine a system where you have more than one GPIO chip.
>
> ...


Yes, thanks for pointing it out. This flag introduced should be a member of
gpio_chip structure as mentioned by Gabriel and then it should work for
multiple gpio chips as well.

>> -       if (gc->to_irq) {
>> +       if (gc->to_irq && gc_irq_initialized) {
>>                  int retirq = gc->to_irq(gc, offset);
> Shouldn't it rather be something like
>
>    if (gc->to_irq) {
>      if (! ..._initialized)
>        return -EPROBE_DEFER;
>      ...
>    }
>
> ?


No, the above code will still bring in issues when gc->to_irq is NULL
and will return -ENXIO. We have seen race in registering of gc->to_irq
as well which is why we had introduced the following patch as the first 
step.

https://lore.kernel.org/lkml/20220216202655.194795-1-shreeya.patel@collabora.com/t/


Thanks,
Shreeya Patel

>
> --
> With Best Regards,
> Andy Shevchenko

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

[Gabriel Krisman Bertazi]

Shreeya Patel <shreeya.patel@collabora.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@collabora.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