Re: [PATCH v5 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing NULL pointer dereference in `esdhc_change_pinstate()` due to insufficient pointer validation.
--

--- Patch [3]: [PATCH v5 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore ---
commit a74e8c023dfc4c92edd0793b792f39383f06ef13
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

This commit moves the IS_ERR checks for pins_100mhz and pins_200mhz into
their respective switch cases. This allows devices with a sleep pinctrl
state but without high-speed pin states to restore their default pin
configuration.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1326,19 +1326,21 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
>  
>  	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>  
> -	if (IS_ERR(imx_data->pinctrl) ||
> -		IS_ERR(imx_data->pins_100mhz) ||
> -		IS_ERR(imx_data->pins_200mhz))
> +	if (IS_ERR(imx_data->pinctrl))
>  		return -EINVAL;
>  
>  	switch (uhs) {
>  	case MMC_TIMING_UHS_SDR50:
>  	case MMC_TIMING_UHS_DDR50:
> +		if (IS_ERR(imx_data->pins_100mhz))

[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer
dereference?

On non-USDHC platforms where the device tree incorrectly advertises UHS
capabilities, pins_100mhz might be uninitialized and left as NULL. Since
IS_ERR() evaluates to false for a NULL pointer, this check will pass,
allowing the NULL pointer to be assigned to pinctrl and later passed to
pinctrl_select_state(), which will panic in pinctrl_commit_state().

Would it be safer to use IS_ERR_OR_NULL() here instead?

> +			return -EINVAL;
>  		pinctrl = imx_data->pins_100mhz;
>  		break;
>  	case MMC_TIMING_UHS_SDR104:
>  	case MMC_TIMING_MMC_HS200:
>  	case MMC_TIMING_MMC_HS400:
> +		if (IS_ERR(imx_data->pins_200mhz))

[Severity: High]
This is also a pre-existing issue, but could the same NULL pointer
dereference happen here?

If pins_200mhz is NULL due to the same misconfigured device tree scenario
mentioned above, this check will also pass, resulting in a panic when NULL
is passed to pinctrl_select_state().

Should this check also be updated to IS_ERR_OR_NULL()?

> +			return -EINVAL;
>  		pinctrl = imx_data->pins_200mhz;
>  		break;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707110303.3779799-1-ziniu.wang_1@oss.nxp.com?part=3