Re: [PATCH v3 3/3] pinctrl: ambarella: add CV75 pin controller

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Signed integer overflow (shift out of bounds) when `offset` is 31.
- [Low] Uninitialized stack variables are consumed if `regmap_read` fails in `amb_pinconf_dbg_show`.
--

--- Patch [3]: [PATCH v3 3/3] pinctrl: ambarella: add CV75 pin controller ---
commit 92a200b443add22149b7a8c8a942fb503b5f5a04
Author: Long Zhao <[email protected]>

pinctrl: ambarella: add CV75 pin controller

Add an Ambarella pinmux-only pinctrl driver with CV75 function/group
tables. GPIO is handled by the PL061 driver.

> diff --git a/drivers/pinctrl/pinctrl-ambarella.c b/drivers/pinctrl/pinctrl-ambarella.c
> index 0000000000000..fb79a8b02209a
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-ambarella.c

[ ... ]

> +static void amb_pinmux_set_altfunc(struct amb_pinctrl_soc_data *soc,
> +				   u32 bank, u32 offset, u32 altfunc)
> +{
> +	u32 i, data;
> +
> +	if (!amb_iomux_accessible(soc))
> +		return;
> +
> +	for (i = 0; i < 3; i++) {
> +		data = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(bank, i));
> +		data &= (~(0x1 << offset));

[Severity: Medium]
Will this cause a signed integer overflow when offset is 31?

Since PINID_TO_OFFSET() masks with 0x1f, offset can be up to 31.
Using 0x1 (a signed 32-bit integer) and shifting it left by 31 results in
undefined behavior and may trigger a UBSAN shift-out-of-bounds splat.

Could this be changed to use 1U or the BIT() macro to avoid the overflow?

> +		data |= (((altfunc >> i) & 0x1) << offset);
> +		writel_relaxed(data, soc->iomux_base + IOMUX_OFFSET(bank, i));
> +	}
> +
> +	amb_iomux_commit(soc);
> +}

[ ... ]

> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static void amb_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> +				 struct seq_file *s, unsigned int pin)
> +{
> +	struct amb_pinctrl_soc_data *soc = pinctrl_dev_get_drvdata(pctldev);
> +	u32 pull_en, pull_dir, ds0, ds1, ds2, ds;
> +	u32 bank, offset;
> +	int strength;
> +
> +	bank = PINID_TO_BANK(pin);
> +	if (bank >= soc->data->nr_banks) {
> +		seq_puts(s, " (no pinconf)");
> +		return;
> +	}
> +
> +	offset = PINID_TO_OFFSET(pin);
> +
> +	regmap_read(soc->pull_regmap, soc->data->pull_en[bank], &pull_en);
> +	pull_en = (pull_en >> offset) & 1;

[Severity: Low]
Does this code consume an uninitialized stack variable if regmap_read() fails?

If the register read fails, pull_en remains uninitialized, and its stack
memory will be consumed in the bitwise operation and eventually printed to
userspace via debugfs.

Should the return values of regmap_read() be checked here to prevent leaking
kernel stack data and triggering KMSAN warnings, similar to how they are
checked in amb_pinconf_get()?

> +	regmap_read(soc->pull_regmap, soc->data->pull_dir[bank], &pull_dir);
> +	pull_dir = (pull_dir >> offset) & 1;
> +	seq_printf(s, " pull: %s,",
> +		   pull_en ? (pull_dir ? "up" : "down") : "disable");

[ ... ]

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