Re: [PATCH v8 7/9] pinctrl: mediatek: mt6397: Add MediaTek MT6392

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] NULL pointer dereference triggered on `gpio_to_irq()` for MT6397 pins.
- [High] Use-After-Free in gpiolib due to missing devres registration and `.remove` callback.
- [High] Use-After-Free in IRQ handler from non-devres chained handler registration.
--

commit b7ca497bf50208e7b29acdf520d006f2e9f79e4b
Author: Luca Leonardo Scorcia <l.scorcia@gmail.com>

pinctrl: mediatek: mt6397: Add MediaTek MT6392
    
Add support for the MT6392 pinctrl device, which is very similar to
MT6397 with a handful of different property values and its own pins
definition.
    
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6397.c b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> index 03d0f65d7bcc1..8ba02e70595ce 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6397.c

[ ... ]

> @@ -40,13 +62,24 @@ static const struct mtk_pinctrl_devdata mt6397_pinctrl_data = {
>  static int mt6397_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct mt6397_chip *mt6397;
> +	const struct mtk_pinctrl_devdata *data;
> +
> +	data = device_get_match_data(&pdev->dev);
> +	if (!data)
> +		return -ENOENT;
>  
>  	mt6397 = dev_get_drvdata(pdev->dev.parent);
> -	return mtk_pctrl_init(pdev, &mt6397_pinctrl_data, mt6397->regmap);
> +	return mtk_pctrl_init(pdev, data, mt6397->regmap);

[Severity: High]
This is a pre-existing issue, but does mtk_pctrl_init() leave pctl->eint
uninitialized for these devices, leading to a NULL pointer dereference?

Because neither mt6392_pinctrl_data nor mt6397_pinctrl_data initialize
the eint_hw field, mtk_pctrl_init() skips mtk_eint_init().

If gpio_to_irq() is later invoked on these pins, it calls down through:

mtk_gpio_to_irq()
   mtk_eint_find_irq(pctl->eint, eint_n)
      irq_find_mapping(eint->domain, eint_n)

Will this dereference the NULL pctl->eint pointer directly?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver lack proper
devres cleanup for the gpiochip?

In mtk_pctrl_init(), pctl->chip is allocated using devm_kzalloc() and
then registered globally:

mtk_pctrl_init()
   pctl->chip = devm_kzalloc(...)
   gpiochip_add_data(pctl->chip, pctl)

Since gpiochip_add_data() does not use devres tracking, and the driver
lacks a .remove callback or suppress_bind_attrs, unbinding the driver
via sysfs will immediately free pctl->chip. Can this leave a dangling
pointer in gpiolib, leading to a use-after-free?

[Severity: High]
This is also a pre-existing issue, but does the IRQ handler registration
capture a devm-allocated pointer without devres tracked cleanup?

For MediaTek pinctrl devices that do initialize EINT, mtk_eint_do_init()
registers the chained handler:

mtk_eint_do_init()
   irq_set_chained_handler_and_data(eint->irq, mtk_eint_irq_handler, eint)

If the driver is unbound, the devm allocated eint structure will be freed.
Does the lack of a cleanup function mean the subsystem could later trigger
mtk_eint_irq_handler() with the freed eint pointer?

>  }
>  
>  static const struct of_device_id mt6397_pctrl_match[] = {
> -	{ .compatible = "mediatek,mt6397-pinctrl", },
> +	{
> +		.compatible = "mediatek,mt6392-pinctrl",
> +		.data = &mt6392_pinctrl_data
> +	}, {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620200032.334192-1-l.scorcia@gmail.com?part=7