Re: [PATCH v2 1/5] pinctrl: qcom: lpass-lpi: make mutex cleanup devm-managed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Data races and Use-After-Free (UAF) vulnerabilities due to early registration and lockless mutation of generic pin groups.
--

--- Patch [1]: [PATCH v2 1/5] pinctrl: qcom: lpass-lpi: make mutex cleanup devm-managed ---
commit 808f67e14184e3ba9ce98d89331c2be909c075e0
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>

pinctrl: qcom: lpass-lpi: make mutex cleanup devm-managed

The driver registers its pin controller using devm_pinctrl_register(),
which keeps the pinctrl device alive until devres teardown, after
.remove() returns. This explicitly replaces mutex_init and
mutex_destroy with devm_mutex_init to align the lifetime automatically.

> diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
> index 5fd4a4eba654d..b3e36547030f8 100644
> --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
> +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
> @@ -559,31 +559,25 @@ int lpi_pinctrl_probe(struct platform_device *pdev)
>  	pctrl->chip.label = dev_name(dev);
>  	pctrl->chip.can_sleep = true;
>  
> -	mutex_init(&pctrl->lock);
> +	ret = devm_mutex_init(&pdev->dev, &pctrl->lock);
> +	if (ret)
> +		return ret;
>  
>  	pctrl->ctrl = devm_pinctrl_register(dev, &pctrl->desc, pctrl);
> -	if (IS_ERR(pctrl->ctrl)) {
> -		ret = PTR_ERR(pctrl->ctrl);
> -		dev_err(dev, "failed to add pin controller\n");
> -		goto err_pinctrl;
> -	}
> +	if (IS_ERR(pctrl->ctrl))
> +		return PTR_ERR(pctrl->ctrl);
>  
>  	ret = lpi_build_pin_desc_groups(pctrl);

[Severity: High]
This is a pre-existing issue, but does this sequence leave the device
vulnerable to data races via debugfs?

When devm_pinctrl_register() is called, it fully registers the device and
exposes it to userspace via debugfs.

Afterward, lpi_build_pin_desc_groups() populates the groups via
pinctrl_generic_add_group(). This modifies pctldev->pin_group_tree and
num_groups without acquiring pctldev->mutex. 

If there is a concurrent debugfs read to the pingroups file, it takes the mutex
and iterates the tree, creating a data race.

>  	if (ret)
> -		goto err_pinctrl;
> +		return ret;
>  
>  	ret = devm_gpiochip_add_data(dev, &pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "can't add gpio chip\n");
> -		goto err_pinctrl;
> +		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but could this error path trigger a
use-after-free?

If devm_gpiochip_add_data() fails, devres unwinds in last-in first-out order.
It will free the group objects (allocated via devm_kzalloc) before executing
devm_pinctrl_dev_release() to unregister the device.

This leaves dangling pointers in the radix tree while debugfs is still active.

>  
>  	return 0;
> -
> -err_pinctrl:
> -	mutex_destroy(&pctrl->lock);
> -
> -	return ret;
>  }
>  EXPORT_SYMBOL_GPL(lpi_pinctrl_probe);
>  
> @@ -592,8 +586,6 @@ void lpi_pinctrl_remove(struct platform_device *pdev)
>  	struct lpi_pinctrl *pctrl = platform_get_drvdata(pdev);
>  	int i;
>  
> -	mutex_destroy(&pctrl->lock);
> -
>  	for (i = 0; i < pctrl->data->npins; i++)
>  		pinctrl_generic_remove_group(pctrl->ctrl, i);

[Severity: High]
This is a pre-existing issue, but can this lockless removal also trigger a
use-after-free?

The driver locklessly calls pinctrl_generic_remove_group(), which deletes
the group from the radix tree and frees it via devm_kfree(). 

Since the device is not unregistered until devres teardown (after remove()
returns), concurrent debugfs reads while this loop is running will
dereference the freed group_desc.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713184549.929569-1-prasad.kumpatla@oss.qualcomm.com?part=1