Re: [PATCH v3] pinctrl: aspeed: Hand over global GPIOP passthrough to local mux controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: aspeed: use-after-free on unbind in aspeed_g6_pinctrl_probe()
--

commit 7926ad91b4f1741a83b80da360b9a845357a5376
Author: Prasanth Kumar Padarthi <prasanth.padarthi10@gmail.com>

pinctrl: aspeed: Hand over global GPIOP passthrough to local mux controls

This commit transfers global GPIOP0-P5 passthrough selections in SCU510 and
SCU51C to per-pin mux controls in SCU4BC during probe, and blocks GPIO and
mux requests on GPIOP0-P5 if the handover fails.

> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> index 85d6640ff4c4e..be22a918420df 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
[ ... ]
> @@ -2778,15 +2827,103 @@ static const struct pinctrl_desc aspeed_g6_pinctrl_desc = {
[ ... ]
>  static int aspeed_g6_pinctrl_probe(struct platform_device *pdev)
>  {
> -	int i;
> +	struct aspeed_g6_probe_data *g6;
> +	struct regmap *scu;
> +	int i, ret;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +	scu = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(scu))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(scu),
> +				     "No SCU regmap for THRU handover\n");
> +
> +	g6 = devm_kzalloc(&pdev->dev, sizeof(*g6), GFP_KERNEL);
> +	if (!g6)
> +		return -ENOMEM;
> +	memcpy(&g6->pdata, &aspeed_g6_pinctrl_data, sizeof(g6->pdata));
> +
> +	ret = aspeed_g6_thru_handover(scu);
> +	if (ret == -EPROBE_DEFER)
> +		return ret;
> +	if (ret) {
> +		g6->thru_error = ret;
> +		dev_warn(&pdev->dev,
> +			 "THRU handover failed (%d); GPIOP0-P5 mux requests blocked\n",
> +			 ret);
> +	}
>  
>  	for (i = 0; i < ARRAY_SIZE(aspeed_g6_pins); i++)
>  		aspeed_g6_pins[i].number = i;
>  
>  	return aspeed_pinctrl_probe(pdev, &aspeed_g6_pinctrl_desc,
> -			&aspeed_g6_pinctrl_data);
> +			&g6->pdata);

[Severity: High]
Can this cause a use-after-free of g6 if the device is unbound?

In aspeed_g6_pinctrl_probe(), g6 is allocated with devm_kzalloc() and
&g6->pdata is passed to aspeed_pinctrl_probe(). However,
drivers/pinctrl/aspeed/pinctrl-aspeed.c:aspeed_pinctrl_probe() registers the
controller with the unmanaged pinctrl_register() helper:

drivers/pinctrl/aspeed/pinctrl-aspeed.c:aspeed_pinctrl_probe() {
	...
	pctl = pinctrl_register(pdesc, &pdev->dev, pdata);
	...
}

Previously, aspeed_g6_pinctrl_probe() passed the static
aspeed_g6_pinctrl_data structure, which remained valid for the lifetime of
the kernel. Because aspeed_g6_pinctrl_driver does not define a .remove
callback to call pinctrl_unregister() and does not set .suppress_bind_attrs,
unbinding the platform device via sysfs frees g6 via devres while the
pinctrl_dev remains registered with driver_data pointing to &g6->pdata.

Subsequent calls to aspeed_g6_set_mux(), aspeed_g6_gpio_request_enable(),
other pinctrl callbacks, or pinctrl debugfs entries will then dereference the
freed g6 structure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105152348.8567-1-prasanth.padarthi10@gmail.com?part=1