Re: [PATCH 2/3] pinctrl: mediatek: Add driver for MT6858

[sashiko-bot@kernel.org]

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

New issues:
- [High] The slew rate (`PINCTRL_PIN_REG_SR`) register range incorrectly aliases the direction (`PINCTRL_PIN_REG_DIR`) register range, corrupting pin input/output state.
- [Low] Directly referencing `mtk_paris_pinctrl_pm_ops` without `pm_sleep_ptr()` causes a build failure when `CONFIG_PM_SLEEP` is disabled.

Pre-existing issues:
- [Critical] The `eint_pins_mt6858` array lacks designated initializers and padding, leading to an out-of-bounds read and deterministic heap corruption during EINT initialization.
- [High] Missing `.remove` callback and `.suppress_bind_attrs = true` causes a Use-After-Free and memory leak on driver unbind.
--

--- Patch [2]: [PATCH 2/3] pinctrl: mediatek: Add driver for MT6858 ---
commit def1c2e865d76b34f711dc474e7d38bf58749658
Author: Nikolai Burov <nikolai.burov@jolla.com>

pinctrl: mediatek: Add driver for MT6858

This commit adds a pinctrl driver for the MT6858 (MediaTek Dimensity 7100)
SoC. It configures pin modes, directions, input/output states, and EINT
hardware mappings.

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6858.c b/drivers/pinctrl/mediatek/pinctrl-mt6858.c
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6858.c

[ ... ]

> @@ -1339,6 +1339,21 @@
>  static const struct mtk_pin_reg_calc mt6858_reg_cals[PINCTRL_PIN_REG_MAX] = {
>  	[PINCTRL_PIN_REG_MODE] = MTK_RANGE(mt6858_pin_mode_range),
>  	[PINCTRL_PIN_REG_DIR] = MTK_RANGE(mt6858_pin_dir_range),
>  	[PINCTRL_PIN_REG_DI] = MTK_RANGE(mt6858_pin_di_range),
>  	[PINCTRL_PIN_REG_DO] = MTK_RANGE(mt6858_pin_do_range),
>  	[PINCTRL_PIN_REG_SR] = MTK_RANGE(mt6858_pin_dir_range),

[Severity: High]
Does this code incorrectly alias the slew rate configuration to the direction
register? 

Setting the PINCTRL_PIN_REG_SR index to MTK_RANGE(mt6858_pin_dir_range)
means that when the device tree specifies PIN_CONFIG_SLEW_RATE, the core
will write to the direction register space. This could inadvertently alter 
the pin's input/output direction instead of the slew rate.

>  	[PINCTRL_PIN_REG_SMT] = MTK_RANGE(mt6858_pin_smt_range),
>  	[PINCTRL_PIN_REG_IES] = MTK_RANGE(mt6858_pin_ies_range),

[ ... ]

> @@ -1360,6 +1375,12 @@
>  static const struct mtk_eint_hw mt6858_eint_hw = {
>  	.port_mask = 0xf,
>  	.ports     = 3,
>  	.ap_num    = 217,

[Severity: Critical]
This is a pre-existing issue, but can this lead to an out-of-bounds read
during initialization? 

In mtk_eint_do_init(), the core loops up to ap_num - 1. Because the 
eint_pins_mt6858 array skips unsupported pins and lacks designated 
initializers, the compiler packs it into exactly 142 elements. Iterating 
up to 216 will read past the array bounds and could corrupt the heap when 
indexing the dynamically allocated pin_list.

>  	.db_cnt    = 36,
>  	.db_time   = debounce_time_mt6878,
>  };

[ ... ]

> @@ -1390,6 +1411,10 @@
>  static struct platform_driver mt6858_pinctrl_driver = {

[Severity: High]
This is a pre-existing issue, but can this cause a use-after-free if the 
driver is unbound via sysfs? 

Since the platform driver lacks a .remove callback and does not set 
.suppress_bind_attrs = true, a privileged user can unbind it. Devres will 
free the hw and eint structures, but the manually created IRQ domain and 
chained handler remain registered. Subsequent hardware interrupts might 
execute the handler and dereference the freed eint pointer.

>  	.driver = {
>  		.name = "mt6858-pinctrl",
>  		.of_match_table = mt6858_pinctrl_of_match,
>  		.pm = &mtk_paris_pinctrl_pm_ops,

[Severity: Low]
Can this cause a link-time build failure? 

The mtk_paris_pinctrl_pm_ops struct is exported using the 
EXPORT_GPL_DEV_SLEEP_PM_OPS() macro, which discards the symbol when 
CONFIG_PM_SLEEP is disabled. Should this reference be wrapped in 
pm_sleep_ptr() to avoid an undefined reference error?

>  	},
>  	.probe = mtk_paris_pinctrl_probe,
>  };

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h b/drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h

[ ... ]

> @@ -2154,6 +2154,10 @@
>  static struct mtk_eint_pin eint_pins_mt6858[] = {
>  	MTK_EINT_PIN(0, 2, 0, 1),
>  	MTK_EINT_PIN(1, 2, 1, 1),

[ ... ]

>  	MTK_EINT_PIN(120, 0, 73, 0),
>  	MTK_EINT_PIN(121, 0, 74, 0),
>  	MTK_EINT_PIN(197, 3, 0, 0),

[Severity: Critical]
This is a pre-existing issue, but can this sequence cause an out-of-bounds
memory access?

The array jumps directly from pin 121 to 197. Without using designated 
initializers, the compiler simply packs the elements sequentially, making 
the array much smaller than the expected ap_num of 217.

>  	MTK_EINT_PIN(198, 3, 1, 0),
>  	MTK_EINT_PIN(199, 3, 2, 0),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-mt6858-pinctrl-v1-0-f75ab558f0df@jolla.com?part=2