Re: [PATCH v2 3/4] mfd: tps65217: Fix irq_domain leak and use-after-free on probe failure

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

New issues:
- [High] Race condition and Use-After-Free in the probe error path due to manual IRQ domain destruction while the parent IRQ is still active.

Pre-existing issues:
- [High] Dynamic modification of a shared global static MFD cell array violates MFD subsystem rules and permanently breaks multi-device bindings.
- [High] Missing validation of `irq_find_mapping()` return value in the interrupt handler causes a kernel oops on unmapped interrupts.
--

--- Patch [3]: [PATCH v2 3/4] mfd: tps65217: Fix irq_domain leak and use-after-free on probe failure ---
commit 1b4bb1d20ba45fa2a7614a8ca5d0feae60617511
Author: Жамбакиев Радий Рикардинович <[email protected]>
Date:   Fri Aug 21 12:37:23 2026 +0000

    mfd: tps65217: Fix irq_domain leak and use-after-free on probe failure
    
    This commit moves the chip revision read ahead of IRQ initialization and
    adds a cleanup helper to dispose of IRQ mappings and remove the irq_domain
    if probe fails. It calls this cleanup helper from the devm_request_threaded_irq
    and devm_mfd_add_devices error paths.

    Cc: [email protected]

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 9a1528456ffc2..d535d140c2e94 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -146,6 +146,24 @@ static const struct irq_domain_ops tps65217_irq_domain_ops = {
>  	.map = tps65217_irq_map,
>  };
>  
> +static void tps65217_irq_cleanup(struct tps65217 *tps)
> +{
> +	unsigned int virq;
> +	int i;
> +
> +	if (!tps->irq_domain)
> +		return;
> +
> +	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
> +		virq = irq_find_mapping(tps->irq_domain, i);

[Severity: High]
This is a pre-existing issue, but irq_find_mapping() can return 0 if a mapping
does not exist.

If a spurious interrupt fires or an interrupt fires during teardown after
mappings are disposed:

drivers/mfd/tps65217.c:tps65217_irq_thread() {
    ...
    handle_nested_irq(irq_find_mapping(tps->irq_domain, i));
    ...
}

Could this pass 0 to handle_nested_irq() and cause a kernel oops when
irq_to_desc(0) returns NULL?

Should the return value of irq_find_mapping() be checked in
tps65217_irq_thread() before passing it to handle_nested_irq()?

> +		if (virq)
> +			irq_dispose_mapping(virq);
> +	}
> +
> +	irq_domain_remove(tps->irq_domain);
> +	tps->irq_domain = NULL;
> +}

[ ... ]

> @@ -337,6 +356,13 @@ static int tps65217_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	ret = tps65217_reg_read(tps, TPS65217_REG_CHIPID, &version);
> +	if (ret < 0) {
> +		dev_err(tps->dev, "Failed to read revision register: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	if (client->irq) {
>  		ret = tps65217_irq_init(tps, client->irq);
>  		if (ret)

[Severity: High]
This is a pre-existing issue, but this code block mutates the shared global
tps65217s array just below the lines modified in this diff:

drivers/mfd/tps65217.c:tps65217_probe() {
    ...
    if (client->irq) {
        ret = tps65217_irq_init(tps, client->irq);
        if (ret)
            return ret;
    } else {
        int i;

        /* Don't tell children about IRQ resources which won't fire */
        for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
            tps65217s[i].num_resources = 0;
    }
    ...
}

Since tps65217s is shared module data, will any subsequent probe of a
TPS65217 device inherit this mutated state and fail to register its IRQ
resources, even if an IRQ is provided?

MFD guidelines require cell arrays to be static const. Should this state
be handled dynamically instead of mutating the static array?

> @@ -354,13 +380,7 @@ static int tps65217_probe(struct i2c_client *client)
>  				   tps->irq_domain);
>  	if (ret < 0) {
>  		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = tps65217_reg_read(tps, TPS65217_REG_CHIPID, &version);
> -	if (ret < 0) {
> -		dev_err(tps->dev, "Failed to read revision register: %d\n",
> -			ret);
> +		tps65217_irq_cleanup(tps);

[Severity: High]
If devm_mfd_add_devices() fails, tps65217_irq_cleanup() manually destroys
tps->irq_domain and sets it to NULL.

Since the parent IRQ was registered via devm_request_threaded_irq() earlier
in probe, it remains active until devres frees it after tps65217_probe()
returns.

If an interrupt fires in this window, could tps65217_irq_thread() locklessly
read the NULL domain, pass it to irq_find_mapping(), and potentially lead
to a kernel oops?

Should the IRQ be explicitly disabled (e.g., via disable_irq()) before
cleaning up the domain?

>  		return ret;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3