Re: [PATCH v2] pinctrl: mediatek: eint: Fix invalid pointer dereference for v1 platforms

Il 21/05/25 15:53, AngeloGioacchino Del Regno ha scritto:

Il 20/05/25 23:15, Nícolas F. R. A. Prado ha scritto:

quoted

Commit 3ef9f710efcb ("pinctrl: mediatek: Add EINT support for multiple
addresses") introduced an access to the 'soc' field of struct
mtk_pinctrl in mtk_eint_do_init() and for that an include of
pinctrl-mtk-common-v2.h.

However, pinctrl drivers relying on the v1 common driver include
pinctrl-mtk-common.h instead, which provides another definition of
struct mtk_pinctrl that does not contain an 'soc' field.

Since mtk_eint_do_init() can be called both by v1 and v2 drivers, it
will now try to dereference an invalid pointer when called on v1
platforms. This has been observed on Genio 350 EVK (MT8365), which
crashes very early in boot (the kernel trace can only be seen with
earlycon).

In order to fix this, since 'struct mtk_pinctrl' was only needed to get
a 'struct mtk_eint_pin', make 'struct mtk_eint_pin' a parameter
of mtk_eint_do_init() so that callers need to supply it, removing
mtk_eint_do_init()'s dependency on any particular 'struct mtk_pinctrl'.

Fixes: 3ef9f710efcb ("pinctrl: mediatek: Add EINT support for multiple addresses")
Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nícolas F. R. A. Prado <redacted>
---
Changes in v2:
- Completely changed approach to make mtk_eint_pin a parameter of
   mtk_eint_do_init() as suggested by Angelo
- Link to v1: https://lore.kernel.org/r/20250519-genio-350-eint-null-ptr-deref- 
fix-v1-1-07445d6d22c3@collabora.com
---
  drivers/pinctrl/mediatek/mtk-eint.c              | 26 ++++++++++--------------
  drivers/pinctrl/mediatek/mtk-eint.h              |  5 +++--
  drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c |  2 +-
  drivers/pinctrl/mediatek/pinctrl-mtk-common.c    |  2 +-
  4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk- 

eint.c
index 
16af6a47028e67bb53db4041a37ebbbb8b9a1e43..d906a5e4101fb10968035fc48e9cf4a444d063a9 100644

--- a/drivers/pinctrl/mediatek/mtk-eint.c
+++ b/drivers/pinctrl/mediatek/mtk-eint.c

@@ -22,7 +22,6 @@

  #include <linux/platform_device.h>
  #include "mtk-eint.h"
-#include "pinctrl-mtk-common-v2.h"
  #define MTK_EINT_EDGE_SENSITIVE           0
  #define MTK_EINT_LEVEL_SENSITIVE          1

@@ -505,10 +504,9 @@ int mtk_eint_find_irq(struct mtk_eint *eint, unsigned long 

eint_n)
  }
  EXPORT_SYMBOL_GPL(mtk_eint_find_irq);
-int mtk_eint_do_init(struct mtk_eint *eint)
+int mtk_eint_do_init(struct mtk_eint *eint, struct mtk_eint_pin *eint_pin)
  {
      unsigned int size, i, port, virq, inst = 0;
-    struct mtk_pinctrl *hw = (struct mtk_pinctrl *)eint->pctl;
      /* If clients don't assign a specific regs, let's use generic one */
      if (!eint->regs)

@@ -519,7 +517,15 @@ int mtk_eint_do_init(struct mtk_eint *eint)

      if (!eint->base_pin_num)
          return -ENOMEM;
-    if (eint->nbase == 1) {

Okay, dropping the nbase == 1 is sane, but that statement was actually documenting
the fact that *eint_pin is used only for multi-base EINT case, so please add those
comments:

quoted

+    if (eint_pin) {

         /* EINT with multiple bases */

quoted

+        eint->pins = eint_pin;
+        for (i = 0; i < eint->hw->ap_num; i++) {
+            inst = eint->pins[i].instance;
+            if (inst >= eint->nbase)
+                continue;
+            eint->base_pin_num[inst]++;
+        }
+    } else {

         /* Single base EINT */

...after which:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Thanks for fixing this!

Just acknowledged that Linus already picked this one.

It's fine either way, no worries.

Cheers,
Angelo