Re: [PATCH 1/2] mfd: tps65217: Fix NULL pointer dereference on IRQ init failure

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

New issues:
- [High] The patch introduces a memory leak and Use-After-Free of the `tps` structure on the probe error path.

Pre-existing issues:
- [High] The patch fails to resolve the NULL pointer dereference and mapping disposal oops for devices configured without an IRQ.
- [High] Devres teardown ordering race causes the IRQ handler to execute with a destroyed IRQ domain.
- [High] Dynamic mutation of the global static `mfd_cell` array corrupts state for multi-instance support and violates MFD guidelines.
- [High] Multi-register password unlock sequences and read-modify-write operations lack driver-level locking.
--

--- Patch [1]: [PATCH 1/2] mfd: tps65217: Fix NULL pointer dereference on IRQ init failure ---
commit dc1e26d9bf54b1760aeb21201e00e49a112f4f13
Author: Жамбакиев Радий Рикардинович <[email protected]>

mfd: tps65217: Fix NULL pointer dereference on IRQ init failure

This commit adds a check for the return value of tps65217_irq_init() in
tps65217_probe() to abort the probe if IRQ initialization fails. This is
intended to prevent a NULL pointer dereference in tps65217_remove() when
the irq_domain creation fails.

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index c240fac0ede76..2d04d9e0ae29c 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -333,7 +333,9 @@ static int tps65217_probe(struct i2c_client *client)
>  	}
>  
>  	if (client->irq) {

[Severity: High]
This is a pre-existing issue, but does this still leave a NULL pointer
dereference and mapping disposal oops if the device is configured without
an IRQ?

If client->irq is 0, tps65217_irq_init() is never called, and tps->irq_domain
remains NULL. When the driver is unbound, tps65217_remove() unconditionally
uses tps->irq_domain:

drivers/mfd/tps65217.c:tps65217_remove() {
    ...
    for (i = 0; i < TPS65217_NUM_IRQ; i++) {
        virq = irq_find_mapping(tps->irq_domain, i);
        if (virq)
            irq_dispose_mapping(virq);
    }

    irq_domain_remove(tps->irq_domain);
    ...
}

Calling irq_find_mapping() with a NULL domain falls back to the default IRQ
domain, which can inappropriately dispose of unrelated mappings. Subsequently,
irq_domain_remove() will oops on the NULL pointer.

[Severity: High]
This is another pre-existing issue related to the IRQ teardown, but is there
a devres ordering race here that could cause the IRQ handler to execute with
a destroyed IRQ domain?

In tps65217_remove(), the IRQ domain is destroyed prematurely before the
threaded IRQ is freed by devres:

drivers/mfd/tps65217.c:tps65217_remove() {
    ...
    irq_domain_remove(tps->irq_domain);
    tps->irq_domain = NULL;
}

Since the interrupt was requested using devm_request_threaded_irq() in
tps65217_irq_init(), its teardown is deferred until devres cleanup runs
after tps65217_remove() returns. If an interrupt fires during this window,
tps65217_irq_thread() runs and calls irq_find_mapping() with a NULL domain,
potentially invoking an unrelated handler or crashing.

> -		tps65217_irq_init(tps, client->irq);
> +		ret = tps65217_irq_init(tps, client->irq);
> +		if (ret)
> +			return ret;

[Severity: High]
Could this new early return introduce a memory leak of the IRQ domain and a
use-after-free of the devres-managed tps structure?

If devm_request_threaded_irq() fails in tps65217_irq_init(), it returns an
error, which is now propagated here. However, the unmanaged IRQ domain
allocated earlier in tps65217_irq_init() via irq_domain_create_linear() is
not freed, causing a leak.

Because probe fails, devres automatically frees the tps structure. The leaked
IRQ domain retains a dangling pointer to this freed structure in
domain->host_data. Any subsequent domain lookups will trigger a
use-after-free.

>  	} else {
>  		int i;

[Severity: High]
This is a pre-existing issue, but does mutating the global static tps65217s
mfd_cell array corrupt state for multi-instance support and violate MFD
guidelines?

A few lines down, if client->irq is 0, tps65217_probe() zeroes out
num_resources for all cells in the globally shared static tps65217s array:

drivers/mfd/tps65217.c:tps65217_probe() {
    ...
    } 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;
    ...
}

If multiple instances of the TPS65217 are probed, an instance without an IRQ
will permanently wipe out the IRQ resources for any subsequent instances that
do have an IRQ. Concurrent probes would also result in a data race.

MFD subsystem guidelines strictly mandate that mfd_cell arrays must be
static const and not dynamically modified.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but do the multi-register password
unlock sequences and read-modify-write operations lack driver-level locking?

Looking at tps65217_reg_write(), it executes a multi-step password unlocking
sequence:

drivers/mfd/tps65217.c:tps65217_reg_write() {
    ...
    xor_reg_val = reg ^ TPS65217_PASSWORD_REGS_UNLOCK;
    ret = regmap_write(tps->regmap, TPS65217_REG_PASSWORD, xor_reg_val);
    if (ret < 0)
        return ret;

    return regmap_write(tps->regmap, reg, val);
}

While regmap protects individual writes, concurrent calls from MFD child
drivers to tps65217_reg_write() or tps65217_update_bits() will interleave.

For example, Thread A's password write could be immediately followed by Thread
B's password write, causing the subsequent target writes to fail or corrupt
other registers.

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