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

[Mehmet Fide <mehmet.fide@gmail.com>]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

> [Severity: High]
> This isn't a bug introduced by this patch, but since you are adding bounds
> checking here, does imx_pinconf_set() also need this verification?
>
> Without validation, a malformed gpio-ranges property in the device tree could
> translate to an invalid pin_id. Since imx_pinconf_set() lacks the
> if (pin_id >= info->npins) check, this unvalidated pin_id could cause an
> out-of-bounds array access in imx_pinconf_set_mmio():
>
> 	const struct imx_pin_reg *pin_reg = &ipctl->pin_regs[pin_id];
>
> Could the same missing bounds check also cause out-of-bounds array accesses to
> ipctl->pin_regs in the GPIO direction callbacks like imx_pmx_gpio_set_direction()
> and vf610_pmx_gpio_set_direction()?

No. Everything that reaches the driver through a gpio range goes through
pinctrl_gpio_request() first, and pin_request() looks the pin up with
pin_desc_get(), which fails for a pin the controller does not have. A bad
gpio-ranges entry therefore fails the GPIO request, and the direction and
config callbacks never see that index.

Which also means the npins check I added in this version is redundant
and its commit message overstates the problem. I can drop it in a v4 or
keep it as a cheap guard, whichever Bartosz and Linus prefer.

> [Severity: High]
> This is another pre-existing issue, but does imx_pinctrl_parse_pin_mmio() also
> lack bounds checking for *pin_id during device tree parsing?
>
> The pin_id is derived algebraically from raw device tree properties:
>
> 	*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 the parsed *pin_id exceeds info->npins, this uses the unverified index
> directly on the ipctl->pin_regs array, leading to an immediate out-of-bounds
> memory write.
>
> Does imx_pinctrl_parse_pin_scu() have a similar vulnerability when indexing the
> info->pins array with an unverified pin->pin?

Yes, that is the one real hole, and it is where the fsl,pins value is
turned into an index. As said on v2, I will send a separate bounds check
for the parsers after this series.

> [Severity: High]
> This is a pre-existing issue, but can concurrent driver probes cause
> hardware state corruption of the shared input_reg in imx_pmx_set_one_pin_mmio()?
>
> There is an unprotected read-modify-write on shared general-purpose IOMUXC
> registers:
>
> 	val = readl(ipctl->base + pin_mmio->input_reg);
> 	val &= ~mask;
> 	val |= select << shift;
> 	writel(val, ipctl->base + pin_mmio->input_reg);
>
> If two drivers concurrently request pinctrl states that share the same
> general-purpose register, and the pinctrl core calls pinmux_enable_setting()
> locklessly, will these RMW operations race and overwrite each other's
> configurations?

The core does call set_mux without a lock and the driver has none around
that read-modify-write, so in principle yes. This series does not touch
the mux path and I have not seen it on Vybrid, so I leave that one to
the i.MX maintainers.

Mehmet