Re: [PATCH] pinctrl: sunxi: keep a shadow copy of the data register output latches

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect calculation of `nbanks` leads to an undersized `dat_shadow` array if `last_pin` is an exact multiple of 32 (or 0 relative to `pin_base`), causing heap out-of-bounds accesses.
--

--- Patch [1]: [PATCH] pinctrl: sunxi: keep a shadow copy of the data register output latches ---
commit 54fd1ee0b54b09d32ab5cbe77eb38c0f817f60a0
Author: Ilya Titov <[email protected]>

pinctrl: sunxi: keep a shadow copy of the data register output latches

This patch introduces a shadow copy of the data registers to avoid corrupting
output latches on input-muxed pins during read-modify-write operations. The
shadow copy is allocated and seeded at probe time.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 25489beeb3125..3cf5cd8ed9914 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
[ ... ]
> @@ -1610,6 +1633,37 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>  	if (!pctl->irq_array)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The bus clock has to be enabled before the pinctrl device
> +	 * registers, as the pin hogs claimed from there access registers.
> +	 */
> +	ret = of_clk_get_parent_count(node);
> +	clk = devm_clk_get_enabled(&pdev->dev, ret == 1 ? NULL : "apb");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	/*
> +	 * Seed the output latch shadow from the hardware so pins the
> +	 * bootloader left in output mode keep their state; see
> +	 * sunxi_pinctrl_gpio_set() for why a shadow is needed.  This must
> +	 * happen before the pinctrl device registers, as pin hogs can mux
> +	 * pins to gpio_out and thereby update the shadow.
> +	 */
> +	last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number;
> +	nbanks = (round_up(last_pin, PINS_PER_BANK) - pctl->desc->pin_base) /
> +		 PINS_PER_BANK;
> +	pctl->dat_shadow = devm_kcalloc(&pdev->dev, nbanks,
> +					sizeof(*pctl->dat_shadow), GFP_KERNEL);

[Severity: High]
Will this nbanks calculation result in an undersized dat_shadow array if
last_pin is an exact multiple of PINS_PER_BANK?

Because last_pin is a 0-indexed maximum pin number, if an SoC's highest pin
number is an exact multiple of 32 (for instance, 32), round_up(32, 32)
evaluates to 32. This would calculate nbanks as 1 instead of 2.

Later in sunxi_pmx_set() or sunxi_pinctrl_gpio_set(), accessing
pctl->dat_shadow[pin / PINS_PER_BANK] could lead to an out-of-bounds heap
access when the pin number evaluates to an index equal to or greater than
nbanks.

> +	if (!pctl->dat_shadow)
> +		return -ENOMEM;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1