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 <[email protected]>
---
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
Shreeya Patel <[email protected]> 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 <[email protected]>
> ---
>
> 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
On 10/03/22 8:14 pm, Andy Shevchenko wrote:
> On Thu, Mar 10, 2022 at 3:22 PM Shreeya Patel
> <[email protected]> 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/[email protected]/t/
Thanks,
Shreeya Patel
>
> --
> With Best Regards,
> Andy Shevchenko
On Thu, Mar 10, 2022 at 3:22 PM Shreeya Patel
<[email protected]> 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