Re: [PATCH v2 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dangerous asymmetry between `pin_config_get` (generic) and `pin_config_set` (raw) causes hardware register corruption.
- [Medium] Incomplete transition to generic pinconf API breaks SCU-based platforms.

Pre-existing issues:
- [Critical] Out-of-bounds write during Device Tree parsing in imx_pinctrl_parse_pin_mmio
--

commit 5ebc05bfe4b54b3133759fc4096ed3eedaf8a185
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register

The mmio pinconf get callback ignores which parameter was requested and
returns the raw conf register, so a generic query through
pinctrl_gpio_get_config() gets register bits back instead of the packed
parameter it asked for. Decode the requested parameter and answer
PIN_CONFIG_OUTPUT_ENABLE and PIN_CONFIG_INPUT_ENABLE on SoCs that declare
where those bits live in the pad register.

> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 9a45b376d36f9..506ee6627c827 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
[ ... ]
> @@ -312,6 +313,38 @@ static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev, unsigned pin_id,
>  	return 0;
>  }
>  
> +static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev,
> +				unsigned int pin_id, unsigned long *config)
> +{
> +	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct imx_pinctrl_soc_info *info = ipctl->info;
> +	const struct imx_pin_reg *pin_reg = &ipctl->pin_regs[pin_id];
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	unsigned int mask;
> +	u32 raw;
> +
> +	/* only the parameters the SoC declares a pad bit for */
> +	switch (param) {
> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +		mask = info->obe_mask;
> +		break;
> +	case PIN_CONFIG_INPUT_ENABLE:
> +		mask = info->ibe_mask;
> +		break;
> +	default:
> +		mask = 0;
> +		break;
> +	}
> +
> +	if (!mask || pin_reg->conf_reg == -1)
> +		return -ENOTSUPP;
> +
> +	raw = readl(ipctl->base + pin_reg->conf_reg);
> +	*config = pinconf_to_config_packed(param, !!(raw & mask));
> +
> +	return 0;
> +}
> +
>  static int imx_pinconf_get(struct pinctrl_dev *pctldev,
>  			   unsigned pin_id, unsigned long *config)
>  {
> @@ -324,6 +357,18 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev,
>  		return imx_pinconf_get_mmio(pctldev, pin_id, config);
>  }

[Severity: Medium]
Does this bypass the generic decoding logic for SCU platforms?

If a generic caller like pinctrl_gpio_get_config() queries an SCU-based
platform, it appears the SCU path (info->imx_pinconf_get) will overwrite
the generic parameter in *config with a raw hardware register value instead
of unpacking it.

> +static int imx_pinconf_get_raw(struct pinctrl_dev *pctldev,
> +			       unsigned int pin_id, unsigned long *config)
> +{
> +	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct imx_pinctrl_soc_info *info = ipctl->info;
> +
> +	if (info->flags & IMX_USE_SCU)
> +		return info->imx_pinconf_get(pctldev, pin_id, config);
> +	else
> +		return imx_pinconf_get_raw_mmio(pctldev, pin_id, config);
> +}
> +
>  static int imx_pinconf_set_mmio(struct pinctrl_dev *pctldev,
>  				unsigned pin_id, unsigned long *configs,
>  				unsigned num_configs)

[Severity: High]
Does this introduce an asymmetry between the get and set callbacks causing
hardware corruption?

While the get callback now unpacks generic parameters, imx_pinconf_set_mmio()
remains completely raw. If a generic caller like pinctrl_gpio_set_config()
passes a packed config (e.g., PIN_CONFIG_LEVEL), will it be blindly written
into the hardware pad configuration register here?

[Severity: Critical]
This is a pre-existing issue, but can the Device Tree parsing cause an
out-of-bounds write here?

Looking at imx_pinctrl_parse_pin_mmio() in drivers/pinctrl/freescale/pinctrl-imx.c:

    *pin_id = (mux_reg != -1) ? mux_reg / 4 : conf_reg / 4;
    pin_reg = &ipctl->pin_regs[*pin_id];
    pin->pin = *pin_id;
    pin_reg->mux_reg = mux_reg;
    pin_reg->conf_reg = conf_reg;

If a maliciously crafted Device Tree overlay provides an excessively large
mux_reg or conf_reg value, the unvalidated pin_id is used to index into
ipctl->pin_regs, potentially overwriting arbitrary memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902073946.3709080-1-mehmet.fide@gmail.com?part=1