Re: [PATCH v10 5/9] mfd: mt6397: Add support for MT6392 PMIC

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Driver-specific header files were added to the global include/linux/mfd/ directory, violating MFD subsystem guidelines.

Pre-existing issues:
- [High] Incomplete resource cleanup in probe error path and missing driver `.remove` callback lead to Use-After-Free and resource leaks.
--

commit cf917ffc4ed0ebfc40bb77132283778e31d7ae09
Author: Fabien Parent <parent.f@gmail.com>

mfd: mt6397: Add support for MT6392 PMIC

Align the MT6397 PMIC driver to other MFD drivers by passing only an
identifier through mt6397_of_match[*].data and add support for the MT6392
PMIC and its regulator, RTC, keys and pinctrl devices.

> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index 3e08179002fad..f284d3885ae18 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
[ ... ]
> @@ -282,6 +317,7 @@ static int mt6397_probe(struct platform_device *pdev)
>  	unsigned int id = 0;
>  	struct mt6397_chip *pmic;
>  	const struct chip_data *pmic_core;
> +	int chip_variant;
>  
>  	pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
>  	if (!pmic)

[Severity: High]
This isn't a bug introduced by this patch, but doesn't the lack of a .remove
callback and incomplete probe error handling leak resources and cause a
use-after-free?

If devm_mfd_add_devices() fails during mt6397_probe(), or if the driver is
unbound via sysfs, devres automatically frees the mt6397_chip structure.
However, mt6397_irq_init() registers a PM notifier:

drivers/mfd/mt6397-irq.c:mt6397_irq_init() {
    ...
    register_pm_notifier(&chip->pm_nb);
    return 0;
}

This is never unregistered because there is no explicit cleanup on the probe
error path, and no .remove callback is defined in mt6397_driver:

drivers/mfd/mt6397-core.c:
static struct platform_driver mt6397_driver = {
    .probe = mt6397_probe,
    .driver = {
        .name = "mt6397",
        .of_match_table = mt6397_of_match,
    },
};

When the system subsequently suspends, the PM core invokes the dangling
notifier_call, causing a UAF.

Additionally, the error path destroys the irq domain while the managed IRQ
thread is still active:

drivers/mfd/mt6397-core.c:mt6397_probe() {
    ...
    if (ret) {
        irq_domain_remove(pmic->irq_domain);
        dev_err(&pdev->dev, "failed to add child devices: %d\n", ret);
    }
    ...
}

Does this create a UAF window if an interrupt fires before devres cleans up
the devm-managed IRQ handler mt6397_irq_thread?

> diff --git a/include/linux/mfd/mt6392/core.h b/include/linux/mfd/mt6392/core.h
> new file mode 100644
> index 0000000000000..8777b3abf9292
> --- /dev/null
> +++ b/include/linux/mfd/mt6392/core.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

[Severity: Low]
Does placing this driver-specific header file in the global include/linux/mfd/
directory violate MFD subsystem layering guidelines?

The MFD subsystem guidelines state: "Do not include driver-specific header
files in the global include/linux/mfd/ directory if they are only used by the
parent and its immediate children. Keep them local to drivers/mfd/."

> diff --git a/include/linux/mfd/mt6392/registers.h b/include/linux/mfd/mt6392/registers.h
> new file mode 100644
> index 0000000000000..68fe9af448f5d
> --- /dev/null
> +++ b/include/linux/mfd/mt6392/registers.h
> @@ -0,0 +1,488 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

[Severity: Low]
Is it intentional to place these driver-specific registers in the global
include directory rather than keeping them local to drivers/mfd/?

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