Re: [PATCH v2 4/4] mfd: tps65217: Fix NULL pointer dereference in remove callback

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

New issues:
- [Medium] The patch unconditionally calls disable_irq_wake() in tps65217_remove(), which will trigger a kernel warning if enable_irq_wake() failed during initialization.

Pre-existing issues:
- [High] The probe error path destroys the IRQ domain while the devres-managed parent IRQ is still active, leading to a race condition and NULL pointer dereference.
- [High] The manual teardown of the IRQ domain in remove() violates devres ordering, leaking child device IRQ actions and threads.
- [High] The probe function dynamically mutates the shared static MFD cell array, permanently corrupting global state.
- [High] tps65217_irq_thread() passes an unchecked return value from irq_find_mapping() to handle_nested_irq(), causing a potential NULL pointer dereference.
--

--- Patch [4]: [PATCH v2 4/4] mfd: tps65217: Fix NULL pointer dereference in remove callback ---
commit bf3143cb9e2d379d490e398c9daa01af3b2614ad
Author: Жамбакиев Радий Рикардинович <[email protected]>

mfd: tps65217: Fix NULL pointer dereference in remove callback

When the device is probed without an interrupt, tps65217_irq_init() is
never called and tps->irq_domain remains NULL. The remove callback
still looks up IRQ mappings and calls irq_domain_remove(), which
dereferences the NULL domain and crashes the kernel. The mapping
lookup with a NULL domain falls back to the default IRQ domain and can
dispose mappings belonging to other devices.

Quiesce the parent interrupt before tearing down the domain: the
devres-managed interrupt is only freed after the remove callback
returns, so an interrupt firing in that window would run the threaded
handler with a NULL irq_domain. Also call disable_irq_wake() to balance
the enable_irq_wake() done in tps65217_irq_init().

Fixes: 6556bdacf646fcaa ("mfd: tps65217: Add support for IRQs")
Cc: [email protected]
Signed-off-by: Radiy Zhambakiev <[email protected]>

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index d535d140c2e94..9f4afbaa65249 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -403,17 +403,20 @@ static int tps65217_probe(struct i2c_client *client)
>  static void tps65217_remove(struct i2c_client *client)
>  {
>  	struct tps65217 *tps = i2c_get_clientdata(client);
> -	unsigned int virq;
> -	int i;
>  
> -	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
> -		virq = irq_find_mapping(tps->irq_domain, i);
> -		if (virq)
> -			irq_dispose_mapping(virq);
> -	}
> +	if (!tps->irq_domain)
> +		return;
>  
> -	irq_domain_remove(tps->irq_domain);
> -	tps->irq_domain = NULL;
> +	/*
> +	 * The interrupt is only freed by devres after this callback
> +	 * returns, so make sure no handler can run while the domain
> +	 * is being torn down.
> +	 */
> +	disable_irq(tps->irq);
> +	synchronize_irq(tps->irq);
> +	disable_irq_wake(tps->irq);

[Severity: Medium]
Will this unconditional disable_irq_wake() cause a kernel warning?

If the parent IRQ controller does not support wakeups, enable_irq_wake() in
tps65217_irq_init() will fail and return an error code, leaving the wake_depth
at 0. Because the driver ignores this return value, it proceeds normally.

When the driver is unloaded, tps65217_remove() unconditionally calls
disable_irq_wake(). This decrements the wake_depth below 0, causing
irq_set_irq_wake() to emit a WARN(1, 'Unbalanced IRQ %d wake disable\n', irq).

> +
> +	tps65217_irq_cleanup(tps);

[Severity: High]
This is a pre-existing issue, but does calling tps65217_irq_cleanup() here leak
child device IRQ actions and threads by violating devres ordering?

Child devices are registered via devm_mfd_add_devices(), meaning they are
unbound by devres after tps65217_remove() returns. However, tps65217_remove()
manually calls tps65217_irq_cleanup(), which disposes IRQ mappings and destroys
the domain.

When devres later unbinds the children, their free_irq() calls attempt to free
IRQs using freed descriptors. irq_to_desc() returns NULL, causing free_irq() to
return silently without freeing the irqaction or stopping the kthread.

>  }


[Severity: High]
This is a pre-existing issue, but does the probe error path destroy the IRQ
domain while the devres-managed parent IRQ is still active?

drivers/mfd/tps65217.c:tps65217_probe() {
    ...
	ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
				   ARRAY_SIZE(tps65217s), NULL, 0,
				   tps->irq_domain);
	if (ret < 0) {
		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
		tps65217_irq_cleanup(tps);
		return ret;
	}
    ...
}

If devm_mfd_add_devices() fails, the error path explicitly calls
tps65217_irq_cleanup(), destroying tps->irq_domain. However, the parent IRQ is
devres-managed and remains active until probe returns.

If the interrupt fires in this window, tps65217_irq_thread() will run with a
NULL irq_domain, passing 0 from irq_find_mapping() into handle_nested_irq(),
which dereferences NULL:

drivers/mfd/tps65217.c:tps65217_irq_thread() {
    ...
	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
		if (status & BIT(i)) {
			handle_nested_irq(irq_find_mapping(tps->irq_domain, i));
			handled = true;
		}
	}
    ...
}


[Severity: High]
This is a pre-existing issue, but does passing the unchecked return value of
irq_find_mapping() to handle_nested_irq() cause a potential NULL pointer
dereference?

drivers/mfd/tps65217.c:tps65217_irq_thread() {
    ...
	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
		if (status & BIT(i)) {
			handle_nested_irq(irq_find_mapping(tps->irq_domain, i));
			handled = true;
		}
	}
    ...
}

irq_find_mapping() returns 0 if it cannot find a mapping for the hardware IRQ.
The driver passes this return value directly to handle_nested_irq(0).

On systems where irq 0 is invalid or sparse IRQs are used, irq_to_desc(0)
returns NULL, leading to a kernel crash when handle_nested_irq() attempts to
lock the descriptor.


[Severity: High]
This is a pre-existing issue, but does dynamically modifying the shared static
tps65217s array permanently corrupt global state?

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;
	}
    ...
}

If a tps65217 device is probed without an interrupt (client->irq == 0), the
probe function loops over the shared static tps65217s array and sets
num_resources = 0 for all cells.

Because this array is global, any subsequently probed devices (even those with
valid IRQs) will inherit this corrupted state and fail to register their
resources.

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