Messages in this thread | | | Subject | Re: [PATCH] gpio: Restrict usage of gc irq members before initialization | From | Shreeya Patel <> | Date | Fri, 11 Mar 2022 19:29:59 +0530 |
| |
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
| |