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

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Adding Linus, Alexander and few others.

On 30/09/26 15:17, sashiko-bot@kernel.org wrote:
> 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.
> 

I switched to pinctrl_generic_pinmux_dt_node_to_map() in v2 as Linus
suggested, so this code is gone.

The generic helper has the same pattern though: on a repeated mapping
the core returns the existing group/function and the newly allocated
copies are never freed. Alexander's series is fixing the same issue
in pinctrl-single and ti-iodelay [0].

Alexander, since your series is trying to fix these issues in other
drivers, could you also include pinctrl-generic.c? It will likely be
used by most new drivers.

Of course, all of this only if Linus and Conor think this is
a valid usecase and the right direction to fix.

Linus, Conor, any thoughts on this?

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

I am not sure this can happen in practice. It needs
pinctrl_generic_add_group() to return an existing selector and
pinmux_generic_add_function() to fail at the same time. If the
function already exists it just returns its selector, so it can only
fail on an allocation failure.

Anyway this code is gone in v2, and the generic helper does not remove
groups in its error path.

> [ ... ]
>> +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.
> 

ti,npins is the number of pins on the pin controller instance, so it
comes from the hardware description like any other DT property. There
is no real upper bound for it that would hold across instances, so I
would rather not add an arbitrary limit in the driver.

ti,proxy-id is already limited to enum: [0, 1] in the binding, so
dtbs_check will catch a wrong value. I will anyway add a check in the
driver as well in v2.

[0]: 
https://lore.kernel.org/all/20261001085217.1778645-1-alexander.sverdlin@siemens.com/

Thanks and regards,
Yemike Abhilash Chandra