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

[Andrea della Porta <andrea.porta@suse.com>]

Hi Wang,

On 07:06 Fri 06 Nov     , 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];

Non-blocking nit-pick: I think that pasting the code in the comment is
too verbose, especially when the same code consists in the entire patch
body here below. A succint explanation by words would be more than enough.

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

Since you're at it, would you mind fixing the SGPIO AON related OOB access too?
Please take a look at bcm2712_[cd]0_aon_gpio_pin_regs struct: they both have
a bunch of AON_SGPIO_REGS macro describing pins at the end of the struct. That
macro puts them at [(n) + 32] in the struct so there's a hole between 'normal'
gpios and SGPIO ones.
Accessing bcm2712_[cd]0_aon_gpio_pin_funcs struct via this +32 offset as the
'pin' index in pc->pin_funcs[pin].funcs[] will result in OOB access. KASAN should
report that if you 'cat pins' in aon pin controller.

One way would be to refactor the code in order to avoid this hole, but I
won't pretend this.
The other way is just add the missing SGPIO entries in bcm2712_[cd]0_aon_gpio_pin_funcs
(mind the +32 offset, a new macro is probably needed).
You can find the functions for SGPIO pins declared in downstream driver, see file
drivers/pinctrl/bcm/pinctrl-bcm2712.c from rpi-6.12.y branch (later branches
already have the upstream driver which is missing the SGPIO functions entirely).
I've posted here the relevant data copied from downstream for your convenience,
please rearrange accordingly:

static const struct bcm2712_pin_funcs bcm2712_c0_aon_sgpio_pin_funcs[] = {
        PIN(0, hdmi_tx0_bsc, hdmi_tx0_auto_i2c, bsc_m0, vc_i2c0, _, _, _, _),
        PIN(1, hdmi_tx0_bsc, hdmi_tx0_auto_i2c, bsc_m0, vc_i2c0, _, _, _, _),
        PIN(2, hdmi_tx1_bsc, hdmi_tx1_auto_i2c, bsc_m1, vc_i2c4, ctl_hdmi_5v, _, _, _),
        PIN(3, hdmi_tx1_bsc, hdmi_tx1_auto_i2c, bsc_m1, vc_i2c4, _, _, _, _),
        PIN(4, avs_pmu_bsc, bsc_m2, vc_i2c5, ctl_hdmi_5v, _, _, _, _),
        PIN(5, avs_pmu_bsc, bsc_m2, vc_i2c5, _, _, _, _, _),
};

static const struct bcm2712_pin_funcs bcm2712_d0_aon_sgpio_pin_funcs[] = {
        PIN(0, hdmi_tx0_bsc, hdmi_tx0_auto_i2c, bsc_m0, vc_i2c0, _, _, _, _),
        PIN(1, hdmi_tx0_bsc, hdmi_tx0_auto_i2c, bsc_m0, vc_i2c0, _, _, _, _),
        PIN(2, hdmi_tx1_bsc, hdmi_tx1_auto_i2c, bsc_m1, vc_i2c0, ctl_hdmi_5v, _, _, _),
        PIN(3, hdmi_tx1_bsc, hdmi_tx1_auto_i2c, bsc_m1, vc_i2c0, _, _, _, _),
        PIN(4, avs_pmu_bsc, bsc_m2, vc_i2c3, ctl_hdmi_5v, _, _, _, _),
        PIN(5, avs_pmu_bsc, bsc_m2, vc_i2c3, _, _, _, _, _),
};

In case you don't want to fix this, you will still have a Reviewed-by from me,
and I'll take care of uploading the fix in a subsequent patch.

Many thanks,
Andrea

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