Re: [PATCH v9 6/6] pinctrl: mediatek: Add MT6735 pinctrl driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pinctrl: mediatek: mt6735: missing pull_type element causes off-by-one and OOB read
- [High] pinctrl: mediatek: mt6735: missing PINCTRL_PIN_REG_MAX bounds creates OOB read vulnerability
- [Low] pinctrl: mediatek: mt6735: inconsistent pin function names using square brackets
--

commit 13f59080a920a07d8352caa06c8ae89603f98199
Author: Yassine Oudjana <y.oudjana@protonmail.com>

pinctrl: mediatek: Add MT6735 pinctrl driver

Add a driver for the MediaTek MT6735 SoC pin controller. This driver
also supports the pin controller on MT6735M, which lacks 6 physical
pins (198-203) used for MSDC2 on MT6735.

diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6735.c b/drivers/pinctrl/mediatek/pinctrl-mt6735.c
--- /dev/null
+++ b/drivers/pinctrl/mediatek/pinctrl-mt6735.c
[ ... ]
> static const struct mtk_pin_reg_calc mt6735_reg_cals[] = {
> 	[PINCTRL_PIN_REG_DIR] = MTK_RANGE(mt6735_pin_dir_range),
> 	[PINCTRL_PIN_REG_DI] = MTK_RANGE(mt6735_pin_di_range),

[Severity: High]
Does this array need to be explicitly sized with PINCTRL_PIN_REG_MAX?

The core validation in mtk_hw_pin_field_get() verifies
field < PINCTRL_PIN_REG_MAX and accesses hw->soc->reg_cal[field].range.
Because this array stops implicitly at the highest initialized enum, reading
an uninitialized or out-of-bounds field can read past the array and cause
an out-of-bounds read vulnerability.

[ ... ]
> static const struct mtk_pin_reg_calc mt6735m_reg_cals[] = {
> 	[PINCTRL_PIN_REG_DIR] = MTK_RANGE(mt6735m_pin_dir_range),
> 	[PINCTRL_PIN_REG_DI] = MTK_RANGE(mt6735m_pin_di_range),

[Severity: High]
Does this array also need explicit sizing with PINCTRL_PIN_REG_MAX to avoid
the same out-of-bounds read vulnerability as mt6735_reg_cals?

[ ... ]
> 	MTK_PULL_PULLSEL_TYPE,		/*  57 */
> 	MTK_PULL_PULLSEL_TYPE,		/*  58 */
> 	MTK_PULL_PULLSEL_TYPE,		/*  60 */
> 	MTK_PULL_PULLSEL_TYPE,		/*  61 */

[Severity: High]
Is index 59 intentionally skipped here?

The mt6735_pull_type array skips index 59, but the corresponding pin
declaration array mtk_pins_mt6735 contains GPIO59. This shift by one causes
the framework to apply the wrong pull configuration rules to all pins from 60
upwards. Also, because the array is undersized by 1, reading the pull type
for the final pin will result in an out-of-bounds read.

[ ... ]
> 	MTK_PULL_PULLSEL_TYPE,		/*  57 */
> 	MTK_PULL_PULLSEL_TYPE,		/*  58 */
> 	MTK_PULL_PULLSEL_TYPE,		/*  60 */
> 	MTK_PULL_PULLSEL_TYPE,		/*  61 */

[Severity: High]
Does the MT6735M pull type array also skip index 59?

This will cause the same off-by-one mapping shift and out-of-bounds read
as seen in the MT6735 array.

diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-mt6735.h b/drivers/pinctrl/mediatek/pinctrl-mtk-mt6735.h
--- /dev/null
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-mt6735.h
[ ... ]
> 	MTK_PIN(
> 		4, "GPIO4",
> 		MTK_EINT_FUNCTION(0, 4),
> 		DRV_GRP2,
> 		MTK_FUNCTION(0, "GPIO4"),
> 		MTK_FUNCTION(1, "CLKM2"),
> 		MTK_FUNCTION(2, "DPI_D8"),
> 		MTK_FUNCTION(3, "SPI_MOB"),
> 		MTK_FUNCTION(4, "TDD_TCK"),
> 		MTK_FUNCTION(5, "CONN_MCU_TCK[0]"),
> 		MTK_FUNCTION(6, "CONN_MCU_AICE_TCKC"),
> 		MTK_FUNCTION(7, "C2K_DM_EINT0")
> 	),
[ ... ]
> 	MTK_PIN(
> 		71, "GPIO71",
> 		MTK_EINT_FUNCTION(0, 71),
> 		DRV_GRP0,
> 		MTK_FUNCTION(0, "GPIO71"),
> 		MTK_FUNCTION(1, "JTCK"),
> 		MTK_FUNCTION(2, "CONN_MCU_TCK[1]"),
> 		MTK_FUNCTION(3, "LTE_MD32_JTAG_TCK"),

[Severity: Low]
Are these square brackets intended in the pin function name?

Square brackets can break identifier parsing conventions in scripts or tools
parsing sysfs/debugfs. Also, the MT6735M variant uses an underscore for
this pin instead:

> 	MTK_PIN(
> 		4, "GPIO4",
> 		MTK_EINT_FUNCTION(0, 4),
> 		DRV_GRP2,
> 		MTK_FUNCTION(0, "GPIO4"),
> 		MTK_FUNCTION(1, "CLKM2"),
> 		MTK_FUNCTION(2, "DPI_D8"),
> 		MTK_FUNCTION(3, "SPI_MOB"),
> 		MTK_FUNCTION(4, "TDD_TCK"),
> 		MTK_FUNCTION(5, "CONN_MCU_TCK_0"),

Can we use standard underscore notation consistently across both variants?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-mt6735-pinctrl-v9-v9-0-bfb19d8bb87c@protonmail.com?part=6