Re: [PATCH v3 04/11] pinctrl: sunxi: add Allwinner B288 pin controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: sunxi: NULL ptr deref on probe deferral due to npins accumulation
--

commit 23f0ad979270766cafcf25e2ddff2cd81bb51adc
Author: Pablo Mazzini <pmazzini@gmail.com>

pinctrl: sunxi: add Allwinner B288 pin controller driver

This commit introduces pin controller and GPIO support for the Allwinner
B288 SoC. It covers configuration for 80 pins across ports B, C, D, F, and
G, including external interrupts.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sun8i-b288.c b/drivers/pinctrl/sunxi/pinctrl-sun8i-b288.c
> new file mode 100644
> index 0000000000000..f973c2b3f2eba
> --- /dev/null
> +++ b/drivers/pinctrl/sunxi/pinctrl-sun8i-b288.c

[ ... ]

> +static struct sunxi_pinctrl_desc b288_pinctrl_data = {
> +	.irq_banks	= 4,
> +};
> +
> +static int b288_pinctrl_probe(struct platform_device *pdev)
> +{
> +	return sunxi_pinctrl_dt_table_init(pdev, b288_nr_bank_pins,
> +					   b288_irq_bank_muxes,
> +					   &b288_pinctrl_data, 0);
> +}

[Severity: High]
If b288_pinctrl_probe() fails and defers, will b288_pinctrl_data.npins
accumulate indefinitely and cause a NULL pointer dereference?

Since b288_pinctrl_data is defined as a static global variable, its state
persists across multiple probe attempts. Looking at
sunxi_pinctrl_dt_table_init(), it forwards this struct to init_pins_table()
which modifies desc->npins:

init_pins_table() {
        ...
        desc->npins += pins_per_bank[i];
        ...
}

If the probe fails and defers, or if the driver is unbound and rebound,
desc->npins is not reset to 0, so its value will double on the next probe
attempt.

This leads to the pins array being allocated for the doubled size, but only
the first 80 elements are actually initialized. The rest are zeroed out by
the allocation. Later, in prepare_function_table(), a loop iterates over
the doubled npins value and calls strcmp(pins[i].pin.name, name). For
indexes beyond the actual number of pins, pins[i].pin.name is NULL, which
triggers a crash.

Should npins be cleared to 0 before initialization, or should the static
struct be treated as read-only and copied to a local device structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005202731.153971-1-pmazzini@gmail.com?part=4