Re: [PATCH 2/3] pinctrl: ti: Add TDA54 pin controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pinctrl: memory leak of devm allocations in dt_node_to_map
- [Medium] pinctrl: erroneous removal of preexisting group on error path
- [Medium] pinctrl: out-of-bounds MMIO access due to unchecked DT properties
--

commit e697f1ea4503df3b1dd2739d4e9326a68689dde7
Author: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>

pinctrl: ti: Add TDA54 pin controller driver

The TDA54 pin controller splits the pad configuration across two registers:
PADCFGA and PADCFGB. This commit adds support for the pin controller in
the TI TDA54 SoC, mapping device tree configurations to pinmux and pinconf
settings.

> diff --git a/drivers/pinctrl/ti/pinctrl-tda54.c b/drivers/pinctrl/ti/pinctrl-tda54.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/ti/pinctrl-tda54.c
[ ... ]
> +static int tda54_dt_node_to_map(struct pinctrl_dev *pctldev,
> +				struct device_node *np,
> +				struct pinctrl_map **maps,
> +				unsigned int *num_maps)
> +{
[ ... ]
> +	grpname = devm_kasprintf(tp->dev, GFP_KERNEL, "%pOFn", np);
> +	pins = devm_kcalloc(tp->dev, npins, sizeof(*pins), GFP_KERNEL);
> +	gdata = devm_kzalloc(tp->dev, sizeof(*gdata), GFP_KERNEL);
> +	grpnames = devm_kcalloc(tp->dev, 1, sizeof(*grpnames), GFP_KERNEL);
> +	if (!grpname || !pins || !gdata || !grpnames) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
[ ... ]
> +	mutex_lock(&tp->mutex);
> +
> +	gsel = pinctrl_generic_add_group(pctldev, grpname, pins, npins, gdata);
> +	if (gsel < 0) {
> +		ret = gsel;
> +		goto unlock;
> +	}

[Severity: High]
Will this leak devm-managed memory if the group already exists?

If a client device defers probe, or if multiple nodes map the same group,
pinctrl_generic_add_group() will return the existing group's selector, which
is a positive integer. Because these buffers are allocated using devm_*
functions tied to the pinctrl device (tp->dev), they will persist until the
pinctrl driver itself is unloaded. This causes a cumulative memory leak on
each probe deferral or repeated group mapping.

> +
> +	grpnames[0] = grpname;
> +	ret = pinmux_generic_add_function(pctldev, grpname, grpnames, 1, NULL);
> +	if (ret < 0)
> +		goto remove_group;
[ ... ]
> +remove_group:
> +	pinctrl_generic_remove_group(pctldev, gsel);

[Severity: Medium]
Could this inadvertently delete a preexisting group in the error path?

If pinctrl_generic_add_group() returned an existing selector and then
pinmux_generic_add_function() fails, the error path jumps to remove_group.
This unconditionally calls pinctrl_generic_remove_group() on the selector,
destroying a preexisting group that might be actively used by other client
devices.

[ ... ]
> +static unsigned int tda54_padcfga(unsigned int pin)
> +{
> +	return TDA54_PADCFGA_BASE + pin * TDA54_PADCFG_STRIDE;
> +}
[ ... ]
> +static int tda54_pinctrl_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	of_property_read_u32(dev->of_node, "ti,proxy-id", &tp->proxy);
> +	tp->base = base + tp->proxy * TDA54_PROXY_STRIDE;
> +
> +	ret = of_property_read_u32(dev->of_node, "ti,npins", &tp->npins);

[Severity: Medium]
Can an excessively large ti,proxy-id or ti,npins value from the device tree
cause out-of-bounds IO memory accesses?

The proxy offset and npins are read directly from the device tree without
being verified against the dynamically mapped IO resource size. Since npins
is used to calculate register offsets like in tda54_padcfga(), an unusually
large value in a malformed device tree could cause the driver to access
memory outside the mapped IO region.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930092020.1047221-1-y-abhilashchandra@ti.com?part=2