Re: [PATCH v1] mfd: mt6397-irq: Fix PM notifier use-after-free

[Lee Jones <lee@kernel.org>]

/* Sashiko Automation: Issues Found (3 Findings) */

Worth considering.

On Sun, 07 Jun 2026, Yuho Choi wrote:

> mt6397_irq_init() registers chip->pm_nb with the global PM notifier
> chain. The notifier callback uses container_of() to recover struct
> mt6397_chip and then dereferences chip fields.
> 
> The chip structure is allocated with devm_kzalloc() in mt6397_probe().
> If probe fails after mt6397_irq_init() succeeds, for example when
> devm_mfd_add_devices() fails, devres can release the chip while the PM
> notifier remains registered. The same lifetime mismatch exists when the
> driver is unbound.
> 
> Check the register_pm_notifier() return value and add a devm cleanup
> action to unregister the notifier before the devm-managed chip is freed.
> If adding the cleanup action fails, devm_add_action_or_reset()
> unregisters the notifier immediately; then remove the IRQ domain in the
> remaining error path.
> 
> Fixes: 4e2e7cfec13a ("mfd: mt6397: Modify suspend/resume behavior")
> Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
> ---
>  drivers/mfd/mt6397-irq.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/mt6397-irq.c b/drivers/mfd/mt6397-irq.c
> index 5d2e5459f744..8947f7e732fa 100644
> --- a/drivers/mfd/mt6397-irq.c
> +++ b/drivers/mfd/mt6397-irq.c
> @@ -169,6 +169,13 @@ static int mt6397_irq_pm_notifier(struct notifier_block *notifier,
>  	return NOTIFY_DONE;
>  }

[Severity: High]
This is a pre-existing issue, but does this PM notifier safely access the
hardware registers and software state?

In mt6397_irq_pm_notifier(), the hardware registers and chip->irq_masks_cur
are read and written without holding chip->irqlock. Since PM notifiers like
PM_SUSPEND_PREPARE run while tasks are still active, if another driver
requests or frees an IRQ concurrently, mt6397_irq_sync_unlock() will acquire
irqlock and write dynamically updated irq_masks_cur to the hardware registers.

Can this data race allow the PM notifier to silently overwrite the dynamically
updated hardware masks with stale data, or corrupt the suspend wake_mask?



>  
> +static void mt6397_irq_pm_notifier_unregister(void *data)
> +{
> +	struct mt6397_chip *chip = data;
> +
> +	unregister_pm_notifier(&chip->pm_nb);
> +}
> +
>  int mt6397_irq_init(struct mt6397_chip *chip)
>  {
>  	int ret;
> @@ -233,6 +240,17 @@ int mt6397_irq_init(struct mt6397_chip *chip)
>  		return ret;
>  	}
>  
> -	register_pm_notifier(&chip->pm_nb);
> -	return 0;
> +	ret = register_pm_notifier(&chip->pm_nb);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to register PM notifier: %d\n", ret);
> +		irq_domain_remove(chip->irq_domain);

[Severity: High]
Is it safe to explicitly remove the IRQ domain here while the devm-managed
parent threaded IRQ remains active?

The parent IRQ was requested earlier via devm_request_threaded_irq(). Because
devres cleanup does not run until the probe function completely fails and
returns, the parent IRQ remains active and enabled during this window.

If an interrupt triggers during this error path, the active IRQ handler
mt6397_irq_thread() will execute and call mt6397_irq_handle_reg(). This in
turn calls irq_find_mapping(), which will dereference the freed irq_domain,
resulting in a use-after-free.



[Severity: High]
This is a pre-existing issue, but does the IRQ domain properly unregister
during driver unbind?

The irq_domain is created using irq_domain_create_linear() earlier in
mt6397_irq_init(), which is not managed by devm. The MT6397 driver lacks a
.remove callback and relies entirely on devres for cleanup.

When the device is unbound, devres frees the chip structure. However, the
irq_domain is never removed and remains in the global IRQ domain list, leaking
memory. Furthermore, its host_data points to the now-freed chip.

If the device is rebound, mt6397_irq_domain_map() will dereference the freed
chip pointer, causing a use-after-free.


> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(chip->dev,
> +				       mt6397_irq_pm_notifier_unregister, chip);
> +	if (ret)
> +		irq_domain_remove(chip->irq_domain);
> +
> +	return ret;
>  }
> -- 
> 2.43.0
> 

-- 
Lee Jones