| Message ID | 20220310132108.225387-1-shreeya.patel@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: Restrict usage of gc irq members before initialization | expand |
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 */
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
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 */
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(-)