Re: [PATCH v4 2/6] pinctrl: qcom: Register functions before enabling pinctrl

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Global function pointer `pm_power_off` and `poweroff_pctrl` state are left pointing to freed memory if probe fails.
--

commit 4cee21ced39b9add5c750146fde36a08b79745d3
Author: Alexandre MINETTE <contact@alex-min.fr>

pinctrl: qcom: Register functions before enabling pinctrl
    
This patch changes the initialization sequence in the Qualcomm pinctrl
driver by registering the pinctrl device with devm_pinctrl_register_and_init,
adding the SoC pin functions, and only then enabling the device to prevent
invalid function map table lookups.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
[ ... ]
> @@ -1593,11 +1593,11 @@ int msm_pinctrl_probe(struct platform_device *pdev,
>  	pctrl->desc.pins = pctrl->soc->pins;
>  	pctrl->desc.npins = pctrl->soc->npins;
>  
> -	pctrl->pctrl = devm_pinctrl_register(&pdev->dev, &pctrl->desc, pctrl);
[ ... ]
> +	ret = devm_pinctrl_register_and_init(&pdev->dev, &pctrl->desc,
> +					     pctrl, &pctrl->pctrl);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Couldn't register pinctrl driver\n");

[Severity: High]
This is a pre-existing issue, but since the failure paths are being
restructured here, what happens to the pm_power_off global pointer if this
registration (or the subsequent pinctrl_enable call) fails?

Earlier in msm_pinctrl_probe(), a call to msm_pinctrl_setup_pm_reset()
assigns the dynamically allocated pctrl to the global poweroff_pctrl pointer
and sets the global pm_power_off to msm_ps_hold_poweroff.

If probe fails here or later:

msm_pinctrl_probe()
    msm_pinctrl_setup_pm_reset(pctrl); 
    ...
    ret = devm_pinctrl_register_and_init(...)
    if (ret)
        return dev_err_probe(...);

The pctrl structure is managed by devres and will be freed upon probe
failure, leaving the global poweroff_pctrl pointing to freed memory. If the
system subsequently initiates a shutdown, msm_ps_hold_poweroff() will
dereference the freed memory, leading to a use-after-free.

Would it make sense to update msm_pinctrl_setup_pm_reset() to use
devm_register_sys_off_handler() with SYS_OFF_MODE_POWER_OFF instead of
directly assigning the pm_power_off global pointer?

>  
>  	for (i = 0; i < soc_data->nfunctions; i++) {
>  		func = &soc_data->functions[i];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-mainline-send-v1-sending-v4-0-83768fbf404d@alex-min.fr?part=2