Re: [PATCH 08/10] pinctrl: mediatek: Clean up partially initialised hw->eint on error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: mediatek: NULL deref in mtk_eint_do_suspend() on EINT init error
--

commit 8d040caade190db874ef6d84f338e06e2679ef73
Author: Lee Jones <lee@kernel.org>

pinctrl: mediatek: Clean up partially initialised hw->eint on error

This commit updates mtk_build_eint() to jump to err_free_base when
of_property_count_strings() fails or hw->eint->nbase is non-positive,
freeing hw->eint and resetting it to NULL.

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> index faa6dbf0f6356..aa15c9e047d89 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> @@ -382,12 +382,16 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	count_reg_names = of_property_count_strings(np, "reg-names");
> -	if (count_reg_names < 0)
> -		return -EINVAL;
> +	if (count_reg_names < 0) {
> +		ret = -EINVAL;
> +		goto err_free_base;
> +	}
>  
>  	hw->eint->nbase = count_reg_names - (int)hw->soc->nbase_names;
> -	if (hw->eint->nbase <= 0)
> -		return -EINVAL;
> +	if (hw->eint->nbase <= 0) {
> +		ret = -EINVAL;
> +		goto err_free_base;
> +	}

[Severity: High]
The commit message notes that leaving hw->eint non-NULL on these error
paths causes subsequent suspend/resume callbacks to dereference a NULL
pointer inside mtk_eint_do_suspend(), and that resetting hw->eint to NULL
avoids this.

However, when mtk_build_eint() fails during mtk_paris_pinctrl_probe(), the
driver logs a warning and finishes probing with pctl->eint set to NULL. On
system suspend or resume, mtk_paris_pinctrl_suspend() and
mtk_paris_pinctrl_resume() pass pctl->eint directly to
mtk_eint_do_suspend() and mtk_eint_do_resume():

drivers/pinctrl/mediatek/pinctrl-paris.c:
static int mtk_paris_pinctrl_suspend(struct device *device)
{
	struct mtk_pinctrl *pctl = dev_get_drvdata(device);

	return mtk_eint_do_suspend(pctl->eint);
}

Neither those callers nor mtk_eint_do_suspend() and mtk_eint_do_resume()
check whether eint is NULL before dereferencing it:

drivers/pinctrl/mediatek/mtk-eint.c:
int mtk_eint_do_suspend(struct mtk_eint *eint)
{
	mtk_eint_chip_write_mask(eint, eint->wake_mask);

	return 0;
}

Previously on these two error paths, hw->eint remained non-NULL with
hw->eint->nbase <= 0, so the loop in mtk_eint_chip_write_mask() over
inst < eint->nbase did zero iterations. With hw->eint reset to NULL here
(as well as on the existing error paths in mtk_build_eint()), won't
suspending or resuming trigger a NULL pointer dereference in
mtk_eint_do_suspend() and mtk_eint_do_resume() unless a NULL check is also
added?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103213002.1673560-1-lee@kernel.org?part=8