[PATCH][5.15.y] gpio: pca953x: fix pca953x_irq_bus_sync_unlock race

From: Ian Ray <ian.ray@gehealthcare.com>

[ Upstream commit bfc6444b57dc7186b6acc964705d7516cbaf3904 ]

Ensure that `i2c_lock' is held when setting interrupt latch and mask in
pca953x_irq_bus_sync_unlock() in order to avoid races.

The other (non-probe) call site pca953x_gpio_set_multiple() ensures the
lock is held before calling pca953x_write_regs().

The problem occurred when a request raced against irq_bus_sync_unlock()
approximately once per thousand reboots on an i.MX8MP based system.

 * Normal case

   0-0022: write register AI|3a {03,02,00,00,01} Input latch P0
   0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
   0-0022: write register AI|08 {ff,00,00,00,00} Output P3
   0-0022: write register AI|12 {fc,00,00,00,00} Config P3

 * Race case

   0-0022: write register AI|08 {ff,00,00,00,00} Output P3
   0-0022: write register AI|08 {03,02,00,00,01} *** Wrong register ***
   0-0022: write register AI|12 {fc,00,00,00,00} Config P3
   0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0

Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
Link: https://lore.kernel.org/r/20240620042915.2173-1-ian.ray@gehealthcare.com
Signed-off-by: Bartosz Golaszewski <redacted>
Signed-off-by: Guocai He <redacted>
---
This commit is to solve the CVE-2024-42253. Please merge this commit to linux-5.15.y.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4860bf3b7e00..4e97b6ae4f72 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c

@@ -672,6 +672,8 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 	int level;
 
 	if (chip->driver_data & PCA_PCAL) {
+		guard(mutex)(&chip->i2c_lock);
+
 		/* Enable latch on interrupt-enabled inputs */
 		pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
 

-- 
2.34.1

Hi Guocai,

On 13/12/24 16:01, guocai.he.cn@windriver.com wrote:

This wouldn't compile on 5.15.y

We don't have scope based locking backported to 5.15.y.


drivers/gpio/gpio-pca953x.c: In function ‘pca953x_irq_bus_sync_unlock’:
drivers/gpio/gpio-pca953x.c:675:17: error: implicit declaration of function ‘guard’ [-Werror=implicit-function-declaration]
  675 |                 guard(mutex)(&chip->i2c_lock);
      |                 ^~~~~
drivers/gpio/gpio-pca953x.c:675:23: error: ‘mutex’ undeclared (first use in this function)
  675 |                 guard(mutex)(&chip->i2c_lock);
      |                       ^~~~~
drivers/gpio/gpio-pca953x.c:675:23: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors



Thanks,
Harshit

On Fri, Dec 13, 2024 at 04:15:09PM +0530, Harshit Mogalapalli wrote:

Hi Guocai,

On 13/12/24 16:01, guocai.he.cn@windriver.com wrote:

quoted

From: Ian Ray <ian.ray@gehealthcare.com>

[ Upstream commit bfc6444b57dc7186b6acc964705d7516cbaf3904 ]

Ensure that `i2c_lock' is held when setting interrupt latch and mask in
pca953x_irq_bus_sync_unlock() in order to avoid races.

The other (non-probe) call site pca953x_gpio_set_multiple() ensures the
lock is held before calling pca953x_write_regs().

The problem occurred when a request raced against irq_bus_sync_unlock()
approximately once per thousand reboots on an i.MX8MP based system.

  * Normal case

    0-0022: write register AI|3a {03,02,00,00,01} Input latch P0
    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
    0-0022: write register AI|12 {fc,00,00,00,00} Config P3

  * Race case

    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
    0-0022: write register AI|08 {03,02,00,00,01} *** Wrong register ***
    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0

Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
Link: https://lore.kernel.org/r/20240620042915.2173-1-ian.ray@gehealthcare.com
Signed-off-by: Bartosz Golaszewski <redacted>
Signed-off-by: Guocai He <redacted>
---
This commit is to solve the CVE-2024-42253. Please merge this commit to linux-5.15.y.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4860bf3b7e00..4e97b6ae4f72 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c

@@ -672,6 +672,8 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
  	int level;
  	if (chip->driver_data & PCA_PCAL) {
+		guard(mutex)(&chip->i2c_lock);

This wouldn't compile on 5.15.y

Which means that no one is actually testing these backports.

Ok, I'm frustrated enough.  No more windriver backports for stable trees
will now be accepted until you all get your act together and figure out
how to do this properly.

As to "how" you prove that you all know what you are doing, I will
leave that up to you to come up with a proper proposal and proof.

ugh.

greg k-h

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: bfc6444b57dc7186b6acc964705d7516cbaf3904

WARNING: Author mismatch between patch and upstream commit:
Backport author: guocai.he.cn@windriver.com
Commit author: Ian Ray [off-list ref]


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.6.y | Present (different SHA1: e2ecdddca80d)
6.1.y | Present (different SHA1: 58a5c93bd1a6)
5.15.y | Not found

Note: The patch differs from the upstream commit:
---
1:  bfc6444b57dc7 ! 1:  3812c0bc93e5e gpio: pca953x: fix pca953x_irq_bus_sync_unlock race
    @@ Metadata
      ## Commit message ##
         gpio: pca953x: fix pca953x_irq_bus_sync_unlock race
     
    +    [ Upstream commit bfc6444b57dc7186b6acc964705d7516cbaf3904 ]
    +
         Ensure that `i2c_lock' is held when setting interrupt latch and mask in
         pca953x_irq_bus_sync_unlock() in order to avoid races.
     
    @@ Commit message
         Signed-off-by: Ian Ray [off-list ref]
         Link: https://lore.kernel.org/r/20240620042915.2173-1-ian.ray@gehealthcare.com
         Signed-off-by: Bartosz Golaszewski [off-list ref]
    +    Signed-off-by: Guocai He [off-list ref]
     
      ## drivers/gpio/gpio-pca953x.c ##
     @@ drivers/gpio/gpio-pca953x.c: static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.15.y       |  Success    |  Failed    |

Build Errors:
Build error for stable/linux-5.15.y:
    drivers/gpio/gpio-pca953x.c: In function 'pca953x_irq_bus_sync_unlock':
    drivers/gpio/gpio-pca953x.c:675:17: error: implicit declaration of function 'guard' [-Werror=implicit-function-declaration]
      675 |                 guard(mutex)(&chip->i2c_lock);
          |                 ^~~~~
    drivers/gpio/gpio-pca953x.c:675:23: error: 'mutex' undeclared (first use in this function)
      675 |                 guard(mutex)(&chip->i2c_lock);
          |                       ^~~~~
    drivers/gpio/gpio-pca953x.c:675:23: note: each undeclared identifier is reported only once for each function it appears in
    cc1: some warnings being treated as errors
    make[2]: *** [scripts/Makefile.build:289: drivers/gpio/gpio-pca953x.o] Error 1
    make[2]: Target '__build' not remade because of errors.
    make[1]: *** [scripts/Makefile.build:552: drivers/gpio] Error 2
    make[1]: Target '__build' not remade because of errors.
    make: *** [Makefile:1906: drivers] Error 2
    make: Target '__all' not remade because of errors.