gpio: Restrict usage of GPIO chip irq members before initialization

GPIO chip 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.

Following are the logs 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: RIP: 0010:gpiochip_to_irq+0x47/0xc0

To avoid such scenarios, restrict usage of GPIO chip irq members before they are completely initialized.

---
 drivers/gpio/gpiolib.c      | 19 +++++++++++++++++++
 include/linux/gpio/driver.h |  9 +++++++++
 2 files changed, 28 insertions(+)

index abdf448b11a3..e4d47e00c392 100644
@@ -2146,6 +2146,16 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	struct irq_domain *domain = chip->irq.domain;
 
+#ifdef CONFIG_GPIOLIB_IRQCHIP
+	/*
+	 * Avoid race condition with other code, which tries to lookup
+	 * an IRQ before the irqchip has been properly registered,
+	 * i.e. while gpiochip is still being brought up.
+	 */
+	if (!chip->irq.initialized)
+		return -EPROBE_DEFER;
+#endif
+
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
 		return -ENXIO;
 

@@ -2321,6 +2331,15 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 
 	acpi_gpiochip_request_interrupts(gpiochip);
 
+	/*
+	 * Using barrier() here to prevent compiler from reordering
+	 * gc->irq.initialized before initialization of above
+	 * GPIO chip irq members.
+	 */
+	barrier();
+
+	gpiochip->irq.initialized = true;
+
 	return 0;
 }
 

index 5dd9c982e2cb..15418caf76fc 100644
@@ -201,6 +201,15 @@ struct gpio_irq_chip {
 	 */
 	bool threaded;
 
+	/**
+	 * @initialized:
+	 *
+	 * Flag to track GPIO chip irq member's initialization.
+	 * This flag will make sure GPIO chip irq members are not used
+	 * before they are initialized.
+	 */
+	bool initialized;
+
 	/**
 	 * @init_hw: optional routine to initialize hardware before
 	 * an IRQ chip will be added. This is quite useful when

2.30.1

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Hi Greg,

Apologies, This is not meant for the Linux kernel. You got spammed because when I cherry-picked this change for canonical only ([email protected]), this got added to the patches:

Which caused it to be sent to everyone.
Will talk to canonical on how to avoid this in the future.

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Friday, February 17, 2023 9:27 AM
To: Asmaa Mnebhi <[email protected]>
Cc: [email protected]; Khoa Vo <[email protected]>; Meriton Tuli <[email protected]>; Vladimir Sokolovsky <[email protected]>; Shreeya Patel <[email protected]>; [email protected]; Andy Shevchenko <[email protected]>; Linus Walleij <[email protected]>; Bartosz Golaszewski <[email protected]>
Subject: Re: [SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

If Canonical just took the normal upstream stable releases (which this commit is already included in), this wouldn't be an issue at all. Why do they not do that?

thanks,

greg k-h

Commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs.

This manifests in messages showing deferred probing while trying to allocate IRQs like so:

amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ .. more of the same .. ]

The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.

Fix this issue by moving the call to acpi_gpiochip_request_interrupts to occur after gc->irc.initialized is set.

---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index e4d47e00c392..049cdfc975b3 100644
@@ -2329,8 +2329,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 
 	gpiochip_set_irq_hooks(gpiochip);
 
-	acpi_gpiochip_request_interrupts(gpiochip);
-
 	/*
 	 * Using barrier() here to prevent compiler from reordering
 	 * gc->irq.initialized before initialization of above

@@ -2340,6 +2338,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 
 	gpiochip->irq.initialized = true;
 
+	acpi_gpiochip_request_interrupts(gpiochip);
+
 	return 0;
 }
 

2.30.1