Re: [PATCH 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 2 potential issue(s) to consider:

New issues:
- [Medium] The patch introduces a read of an uninitialized stack variable, causing non-deterministic behavior and a cascading state trap that corrupts debugfs output.

Pre-existing issues:
- [High] Generic queries for unsupported SoCs fall through to the legacy path and return raw register bits instead of -ENOTSUPP, violating the generic API contract.
--

commit b3256a64d0ea28cb67caa6371dfdc4e8b8b48f42
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.

> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 9a45b376d36f9..5da1a76b01412 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c

[ ... ]

> @@ -297,6 +298,35 @@ static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev, unsigned pin_id,
>  	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 = 0;
> +	u32 raw;
> +
> +	/*
> +	 * Only OUTPUT_ENABLE/INPUT_ENABLE are decoded, and only when the SoC
> +	 * declares the bits. Everything else still returns the raw conf
> +	 * register, the debugfs dump depends on it.
> +	 */
> +	switch (param) {
> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +		mask = info->obe_mask;
> +		break;
> +	case PIN_CONFIG_INPUT_ENABLE:
> +		mask = info->ibe_mask;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (mask) {
> +		/* pin not configured, nothing to report */
> +		if (pin_reg->conf_reg == -1)
> +			return -ENOTSUPP;
> +
> +		raw = readl(ipctl->base + pin_reg->conf_reg);
> +		*config = pinconf_to_config_packed(param, !!(raw & mask));

[Severity: Medium]
Is it safe to extract a parameter from the incoming *config pointer?

Looking at imx_pinconf_group_dbg_show() in the same file, config is passed
to imx_pinconf_get() uninitialized:

drivers/pinctrl/freescale/pinctrl-imx.c:imx_pinconf_group_dbg_show() {
	...
	unsigned long config;
	...
	for (i = 0; i < grp->grp.npins; i++) {
		...
		ret = imx_pinconf_get(pctldev, pin->pin, &config);
	...
}

Since config contains stack garbage on the first iteration, and its
value is retained across loop iterations, could pinconf_to_config_param()
randomly decode into PIN_CONFIG_OUTPUT_ENABLE or PIN_CONFIG_INPUT_ENABLE?

If it does, or if a previous pin's raw register ends in the same byte,
won't this path embed the parameter back into the lowest 8 bits via
pinconf_to_config_packed()?

This appears to permanently trap all subsequent loop iterations into falsely
triggering the generic decoding path instead of returning raw registers.

> +		return 0;
> +	}
>  
>  	if (pin_reg->conf_reg == -1) {
>  		dev_err(ipctl->dev, "Pin(%s) does not support config function\n",

[Severity: High]
This is a pre-existing issue, but does this correctly handle generic queries
on SoCs that lack the requested masks?

If a generic caller requests PIN_CONFIG_OUTPUT_ENABLE via
pinctrl_gpio_get_config(), and the SoC does not define obe_mask, the
mask remains 0 and bypasses the if (mask) decoding block.

Does this cause the query to fall through to the default path:

drivers/pinctrl/freescale/pinctrl-imx.c:imx_pinconf_get_mmio() {
	...
	*config = readl(ipctl->base + pin_reg->conf_reg);

	if (info->flags & SHARE_MUX_CONF_REG)
		*config &= ~info->mux_mask;

	return 0;
}

and succeed, returning raw pad register bits to a generic caller instead of
rejecting the unsupported generic parameter with -ENOTSUPP?

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