[SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

[SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

[Asmaa Mnebhi]

BugLink: https://bugs.launchpad.net/bugs/2007581

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.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Cc: stable@vger.kernel.org
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
(backported from commit 5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320)
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/gpiolib.c      | 19 +++++++++++++++++++
 include/linux/gpio/driver.h |  9 +++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index abdf448b11a3..e4d47e00c392 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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;
 }
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 5dd9c982e2cb..15418caf76fc 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -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

Re: [SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

[Greg KH]

On Fri, Feb 17, 2023 at 09:07:43AM -0500, Asmaa Mnebhi wrote:
> BugLink: https://bugs.launchpad.net/bugs/2007581
> 
> 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.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> (backported from commit 5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320)
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>


<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>

RE: [SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

[Asmaa Mnebhi]

Hi Greg, 

Apologies, This is not meant for the Linux kernel. You got spammed because when I cherry-picked this change for canonical only (kernel-team@lists.ubuntu.com), this got added to the patches:
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> (backported from 
> commit 5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320)
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>

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 <gregkh@linuxfoundation.org> 
Sent: Friday, February 17, 2023 9:27 AM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: kernel-team@lists.ubuntu.com; Khoa Vo <khoav@nvidia.com>; Meriton Tuli <meriton@nvidia.com>; Vladimir Sokolovsky <vlad@nvidia.com>; Shreeya Patel <shreeya.patel@collabora.com>; stable@vger.kernel.org; Andy Shevchenko <andy.shevchenko@gmail.com>; Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>
Subject: Re: [SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

On Fri, Feb 17, 2023 at 09:07:43AM -0500, Asmaa Mnebhi wrote:
> BugLink: https://bugs.launchpad.net/bugs/2007581
> 
> 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.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> (backported from 
> commit 5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320)
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>


<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>

Re: [SRU][F:linux-bluefield][PATCH v2 1/2] gpio: Restrict usage of GPIO chip irq members before initialization

[Greg KH]

On Fri, Feb 17, 2023 at 03:33:28PM +0000, Asmaa Mnebhi wrote:
> Hi Greg, 
> 
> Apologies, This is not meant for the Linux kernel. You got spammed because when I cherry-picked this change for canonical only (kernel-team@lists.ubuntu.com), this got added to the patches:
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> (backported from 
> > commit 5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320)
> > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> 
> Which caused it to be sent to everyone.  
> Will talk to canonical on how to avoid this in the future.

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

[SRU][F:linux-bluefield][PATCH v2 2/2] gpio: Request interrupts after IRQ is initialized

[Asmaa Mnebhi]

BugLink: https://bugs.launchpad.net/bugs/2007581

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.

Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization")
Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL1PR12MB5157.namprd12.prod.outlook.com/
Link: https://bugzilla.suse.com/show_bug.cgi?id=1198697
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215850
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1979
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1976
Reported-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Shreeya Patel <shreeya.patel@collabora.com>
Tested-By: Samuel Čavoj <samuel@cavoj.net>
Tested-By: lukeluk498@gmail.com Link:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-and-tested-by: Takashi Iwai <tiwai@suse.de>
Cc: Shreeya Patel <shreeya.patel@collabora.com>
Cc: stable@vger.kernel.org
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(backported from commit 06fb4ecfeac7e00d6704fa5ed19299f2fefb3cc9)
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e4d47e00c392..049cdfc975b3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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