[PATCH 0/3] pinctrl: bcm: Fix fsel_get() and use generic dt_free_map

[PATCH 0/3] pinctrl: bcm: Fix fsel_get() and use generic dt_free_map

[Wang Xiaolei]

This series contains one fix and two cleanups for the Broadcom pin
controllers.

Patch 1 fixes an off-by-one and out-of-bounds read in the STB pin
controller driver that was added in v6.18. The FSEL register field is
a 1-based function number -- 0 means GPIO, 1..n_funcs select
funcs[0..n_funcs - 1] -- but brcmstb_pinctrl_fsel_get() uses the raw
field value as a 0-based index into funcs[]. Every readback is
therefore shifted by one entry, and the value 8, which
brcmstb_pinctrl_fsel_set() accepts as the eighth alternate function,
reads funcs[8] and trips KASAN when the pin state is printed through
debugfs.

Patches 2 and 3 replace hand-written dt_free_map callbacks with
pinconf_generic_dt_free_map(). Both drivers already call
pinconf_generic_dt_node_to_map_*() for dt_node_to_map and already
select GENERIC_PINCONF, so the generic helper is the matching
counterpart, and the open-coded free loop just duplicates what
pinctrl_utils_free_map() already does. No functional change is
intended for the map types the existing device trees use.

Patch 1 was tested on a Raspberry Pi 5 with KASAN enabled. Before the
change, writing "gpio45 alt8" to pinmux-select and then reading pins
triggers a global-out-of-bounds splat in brcmstb_pctl_pin_dbg_show();
after the change the same sequence reports "function spi_m" without
a warning, and a register field value of 0 reports "gpio".

The series is based on v7.4-rc1 and has no inter-patch dependencies.


Wang Xiaolei (3):
  pinctrl: bcm: Fix off-by-one and out-of-bounds access in fsel_get()
  pinctrl: bcm: Use pinconf_generic_dt_free_map in bcm2835
  pinctrl: bcm: Use pinconf_generic_dt_free_map in brcmstb

 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 16 ++--------------
 drivers/pinctrl/bcm/pinctrl-brcmstb.c | 26 +++++++++++---------------
 2 files changed, 13 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH 1/3] pinctrl: bcm: Fix off-by-one and out-of-bounds access in fsel_get()

[Wang Xiaolei]

brcmstb_pinctrl_fsel_get() takes the value read from the pin's FSEL
register and uses it directly as an index into
pc->pin_funcs[pin].funcs[]:

        fsel = (val >> BIT_TO_SHIFT(bit)) & pc->pin_funcs[pin].func_mask;
        func = pc->pin_funcs[pin].funcs[fsel];

That is not how the register field is encoded. The setter,
brcmstb_pinctrl_fsel_set(), treats 0 as GPIO, 1..n_funcs as
funcs[0..n_funcs - 1], rejects n_funcs + 1 and above, and n_funcs is
always BRCMSTB_FSEL_COUNT (8). The field is therefore a 1-based function
number, while funcs[] is a 0-based array of eight entries.

Every readback is consequently shifted by one slot, and a field value of
8 -- which the setter accepts, being the eighth and last alternate
function -- indexes funcs[8], one element past the end of the array.

This is reachable from userspace without supplying any invalid input:

        # echo "gpio45 alt8" > .../pinmux-select
        # cat .../pins

The write is an ordinary mux request that fsel_set() accepts; the read
that follows goes through pinctrl_pins_show() ->
brcmstb_pctl_pin_dbg_show() -> fsel_get() and triggers KASAN on
Raspberry Pi 5:

        BUG: KASAN: global-out-of-bounds in brcmstb_pctl_pin_dbg_show
        Read of size 1 at addr ffffa07c577ab2e8 by task cat/839
        The buggy address belongs to the variable:
         __compound_literal.62+0x8/0x40

A field value of 0 has the same root cause in a milder form: it is
reported as funcs[0] instead of GPIO. The mux itself is programmed
correctly in both cases, so this affects the debugfs readback and KASAN
rather than the hardware configuration.

Handle 0 and out-of-range values explicitly, and index funcs[] with
fsel - 1, so that the getter uses the same numbering as the setter and
the index can no longer go past funcs[n_funcs - 1]. The existing
fallback for func__ placeholder entries is kept:

        if (!fsel)
                return pc->func_gpio;

        if (fsel > pc->pin_funcs[pin].n_funcs) {
                dev_warn(pc->dev,
			 "invalid fsel %d for pin %u\n", fsel, pin);
                return pc->func_gpio;
        }

        func = pc->pin_funcs[pin].funcs[fsel - 1];

Tested on Raspberry Pi 5 with KASAN enabled. Before the fix, writing
"gpio45 alt8" to .../pinmux-select and then reading .../pins triggers
the splat above. After the fix the same sequence reports
"function spi_m" with no warning and no splat, and a register field
value of 0 reports "gpio". The register is restored to 0 afterwards.

Fixes: 657cbf9b24ba ("pinctrl: bcm: Add STB family pin controller driver")
Cc: Jonathan Bell <jonathan@raspberrypi.com>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Wang Xiaolei <wangxiaolei01@kaihong.com>
---
 drivers/pinctrl/bcm/pinctrl-brcmstb.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-brcmstb.c b/drivers/pinctrl/bcm/pinctrl-brcmstb.c
index f46b27155c3c..2b942ca7196b 100644
--- a/drivers/pinctrl/bcm/pinctrl-brcmstb.c
+++ b/drivers/pinctrl/bcm/pinctrl-brcmstb.c
@@ -66,7 +66,16 @@ static unsigned int brcmstb_pinctrl_fsel_get(struct brcmstb_pinctrl *pc,
 
 	val = readl(pc->base + BIT_TO_REG(bit));
 	fsel = (val >> BIT_TO_SHIFT(bit)) & pc->pin_funcs[pin].func_mask;
-	func = pc->pin_funcs[pin].funcs[fsel];
+
+	if (!fsel)
+		return pc->func_gpio;
+
+	if (fsel > pc->pin_funcs[pin].n_funcs) {
+		dev_warn(pc->dev, "invalid fsel %d for pin %u\n", fsel, pin);
+		return pc->func_gpio;
+	}
+
+	func = pc->pin_funcs[pin].funcs[fsel - 1];
 
 	if (func >= pc->func_count)
 		func = fsel;
-- 
2.34.1

Re: [PATCH 1/3] pinctrl: bcm: Fix off-by-one and out-of-bounds access in fsel_get()

[Florian Fainelli]

On 11/5/26 23:06, Wang Xiaolei wrote:
> brcmstb_pinctrl_fsel_get() takes the value read from the pin's FSEL
> register and uses it directly as an index into
> pc->pin_funcs[pin].funcs[]:
> 
>          fsel = (val >> BIT_TO_SHIFT(bit)) & pc->pin_funcs[pin].func_mask;
>          func = pc->pin_funcs[pin].funcs[fsel];
> 
> That is not how the register field is encoded. The setter,
> brcmstb_pinctrl_fsel_set(), treats 0 as GPIO, 1..n_funcs as
> funcs[0..n_funcs - 1], rejects n_funcs + 1 and above, and n_funcs is
> always BRCMSTB_FSEL_COUNT (8). The field is therefore a 1-based function
> number, while funcs[] is a 0-based array of eight entries.
> 
> Every readback is consequently shifted by one slot, and a field value of
> 8 -- which the setter accepts, being the eighth and last alternate
> function -- indexes funcs[8], one element past the end of the array.
> 
> This is reachable from userspace without supplying any invalid input:
> 
>          # echo "gpio45 alt8" > .../pinmux-select
>          # cat .../pins
> 
> The write is an ordinary mux request that fsel_set() accepts; the read
> that follows goes through pinctrl_pins_show() ->
> brcmstb_pctl_pin_dbg_show() -> fsel_get() and triggers KASAN on
> Raspberry Pi 5:
> 
>          BUG: KASAN: global-out-of-bounds in brcmstb_pctl_pin_dbg_show
>          Read of size 1 at addr ffffa07c577ab2e8 by task cat/839
>          The buggy address belongs to the variable:
>           __compound_literal.62+0x8/0x40
> 
> A field value of 0 has the same root cause in a milder form: it is
> reported as funcs[0] instead of GPIO. The mux itself is programmed
> correctly in both cases, so this affects the debugfs readback and KASAN
> rather than the hardware configuration.
> 
> Handle 0 and out-of-range values explicitly, and index funcs[] with
> fsel - 1, so that the getter uses the same numbering as the setter and
> the index can no longer go past funcs[n_funcs - 1]. The existing
> fallback for func__ placeholder entries is kept:
> 
>          if (!fsel)
>                  return pc->func_gpio;
> 
>          if (fsel > pc->pin_funcs[pin].n_funcs) {
>                  dev_warn(pc->dev,
> 			 "invalid fsel %d for pin %u\n", fsel, pin);
>                  return pc->func_gpio;
>          }
> 
>          func = pc->pin_funcs[pin].funcs[fsel - 1];
> 
> Tested on Raspberry Pi 5 with KASAN enabled. Before the fix, writing
> "gpio45 alt8" to .../pinmux-select and then reading .../pins triggers
> the splat above. After the fix the same sequence reports
> "function spi_m" with no warning and no splat, and a register field
> value of 0 reports "gpio". The register is restored to 0 afterwards.
> 
> Fixes: 657cbf9b24ba ("pinctrl: bcm: Add STB family pin controller driver")
> Cc: Jonathan Bell <jonathan@raspberrypi.com>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Wang Xiaolei <wangxiaolei01@kaihong.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH 2/3] pinctrl: bcm: Use pinconf_generic_dt_free_map in bcm2835

[Wang Xiaolei]

Replace the hand-written dt_free_map callback with the generic
pinconf_generic_dt_free_map() helper, which wraps
pinctrl_utils_free_map(). This drops the duplicated free loop and
handles both CONFIGS_PIN and CONFIGS_GROUP map types.

The error path in bcm2835_pctl_dt_node_to_map() uses the same helper.
The driver already selects GENERIC_PINCONF, so no Kconfig change is
needed.

Signed-off-by: Wang Xiaolei <wangxiaolei01@kaihong.com>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 725e880ae086..69a92cd7bfe0 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -760,18 +760,6 @@ static void bcm2835_pctl_pin_dbg_show(struct pinctrl_dev *pctldev,
 		irq, irq_type_names[pc->irq_type[offset]]);
 }
 
-static void bcm2835_pctl_dt_free_map(struct pinctrl_dev *pctldev,
-		struct pinctrl_map *maps, unsigned num_maps)
-{
-	int i;
-
-	for (i = 0; i < num_maps; i++)
-		if (maps[i].type == PIN_MAP_TYPE_CONFIGS_PIN)
-			kfree(maps[i].data.configs.configs);
-
-	kfree(maps);
-}
-
 static int bcm2835_pctl_dt_node_to_map_func(struct bcm2835_pinctrl *pc,
 		struct device_node *np, u32 pin, u32 fnum,
 		struct pinctrl_map **maps)
@@ -916,7 +904,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	return 0;
 
 out:
-	bcm2835_pctl_dt_free_map(pctldev, maps, num_pins * maps_per_pin);
+	pinconf_generic_dt_free_map(pctldev, maps, num_pins * maps_per_pin);
 	return err;
 }
 
@@ -926,7 +914,7 @@ static const struct pinctrl_ops bcm2835_pctl_ops = {
 	.get_group_pins = bcm2835_pctl_get_group_pins,
 	.pin_dbg_show = bcm2835_pctl_pin_dbg_show,
 	.dt_node_to_map = bcm2835_pctl_dt_node_to_map,
-	.dt_free_map = bcm2835_pctl_dt_free_map,
+	.dt_free_map = pinconf_generic_dt_free_map,
 };
 
 static int bcm2835_pmx_free(struct pinctrl_dev *pctldev,
-- 
2.34.1

Re: [PATCH 2/3] pinctrl: bcm: Use pinconf_generic_dt_free_map in bcm2835

[Florian Fainelli]

On 11/5/26 23:06, Wang Xiaolei wrote:
> Replace the hand-written dt_free_map callback with the generic
> pinconf_generic_dt_free_map() helper, which wraps
> pinctrl_utils_free_map(). This drops the duplicated free loop and
> handles both CONFIGS_PIN and CONFIGS_GROUP map types.
> 
> The error path in bcm2835_pctl_dt_node_to_map() uses the same helper.
> The driver already selects GENERIC_PINCONF, so no Kconfig change is
> needed.
> 
> Signed-off-by: Wang Xiaolei <wangxiaolei01@kaihong.com>

I think this deserves a Fixes: tag because it fixes a memory leak, 
generic allocation can allocate PIN_MAP_TYPE_CONFIGS_GROUP, but the 
custom callback does not free that allocation type:

Fixes: 0de704955ee4 ("pinctrl: bcm2835: Add support for generic pinctrl 
binding")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH 3/3] pinctrl: bcm: Use pinconf_generic_dt_free_map in brcmstb

[Wang Xiaolei]

Replace the hand-written dt_free_map callback with the generic
pinconf_generic_dt_free_map() helper, which wraps
pinctrl_utils_free_map(). This drops the duplicated free loop and
pairs the callback with the pinconf_generic_dt_node_to_map_all()
already used for dt_node_to_map.

The driver already selects GENERIC_PINCONF, so no Kconfig change is
needed.

Cc: Jonathan Bell <jonathan@raspberrypi.com>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Wang Xiaolei <wangxiaolei01@kaihong.com>
---
 drivers/pinctrl/bcm/pinctrl-brcmstb.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-brcmstb.c b/drivers/pinctrl/bcm/pinctrl-brcmstb.c
index 2b942ca7196b..e81c5b295a05 100644
--- a/drivers/pinctrl/bcm/pinctrl-brcmstb.c
+++ b/drivers/pinctrl/bcm/pinctrl-brcmstb.c
@@ -180,26 +180,13 @@ static void brcmstb_pctl_pin_dbg_show(struct pinctrl_dev *pctldev,
 	seq_printf(s, "function %s", fname);
 }
 
-static void brcmstb_pctl_dt_free_map(struct pinctrl_dev *pctldev,
-				     struct pinctrl_map *maps,
-				     unsigned int num_maps)
-{
-	int i;
-
-	for (i = 0; i < num_maps; i++)
-		if (maps[i].type == PIN_MAP_TYPE_CONFIGS_PIN)
-			kfree(maps[i].data.configs.configs);
-
-	kfree(maps);
-}
-
 static const struct pinctrl_ops brcmstb_pctl_ops = {
 	.get_groups_count = brcmstb_pctl_get_groups_count,
 	.get_group_name = brcmstb_pctl_get_group_name,
 	.get_group_pins = brcmstb_pctl_get_group_pins,
 	.pin_dbg_show = brcmstb_pctl_pin_dbg_show,
 	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
-	.dt_free_map = brcmstb_pctl_dt_free_map,
+	.dt_free_map = pinconf_generic_dt_free_map,
 };
 
 static int brcmstb_pmx_free(struct pinctrl_dev *pctldev, unsigned int offset)
-- 
2.34.1

Re: [PATCH 3/3] pinctrl: bcm: Use pinconf_generic_dt_free_map in brcmstb

[Florian Fainelli]

On 11/5/26 23:06, Wang Xiaolei wrote:
> Replace the hand-written dt_free_map callback with the generic
> pinconf_generic_dt_free_map() helper, which wraps
> pinctrl_utils_free_map(). This drops the duplicated free loop and
> pairs the callback with the pinconf_generic_dt_node_to_map_all()
> already used for dt_node_to_map.
> 
> The driver already selects GENERIC_PINCONF, so no Kconfig change is
> needed.
> 
> Cc: Jonathan Bell <jonathan@raspberrypi.com>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Wang Xiaolei <wangxiaolei01@kaihong.com>

Similar thing to patch #2, this should have:

Fixes: 657cbf9b24ba ("pinctrl: bcm: Add STB family pin controller driver")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian