[PATCH v2 0/4] mfd: tps65217: Handle IRQ initialization errors

[PATCH v2 0/4] mfd: tps65217: Handle IRQ initialization errors

[Жамбакиев Радий Рикардинович]

This series fixes the error handling in the IRQ setup and teardown
paths of the TPS65217 MFD driver.

Patch 1 propagates the error from tps65217_irq_init() in probe, so a
failed irq_domain creation does not leave a NULL domain behind that
tps65217_remove() would later dereference.
Patch 2 propagates the error of the initial register write in
tps65217_irq_init(), so a failed mask write cannot leave the software
mask out of sync with the hardware.
Patch 3 fixes the irq_domain leak and the resulting 
use-after-free when probe fails after the domain has been created.
Patch 4 fixes the NULL pointer dereference in the remove callback on
devices probed without an interrupt, quiesces the parent interrupt
before the domain is torn down, and balances enable_irq_wake().

Changes since v1:
- added "mfd: tps65217: Fix irq_domain leak and use-after-free on probe
  failure"
- added "mfd: tps65217: Fix NULL pointer dereference in remove callback"

Patches 3 and 4 fix the in-scope findings of the Sashiko AI review.
The remaining findings, in my opinion, are out of scope for this series and may be
addressed in separate follow-up patches.

Radiy Zhambakiev (4):
  mfd: tps65217: Fix NULL pointer dereference on IRQ init failure
  mfd: tps65217: Check return value when masking interrupt sources
  mfd: tps65217: Fix irq_domain leak and use-after-free on probe failure
  mfd: tps65217: Fix NULL pointer dereference in remove callback

 drivers/mfd/tps65217.c | 68 ++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 19 deletions(-)

-- 
2.53.0

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

[Жамбакиев Радий Рикардинович]

tps65217_probe() ignores the return value of tps65217_irq_init(), so
when the irq domain creation fails the probe still completes and the
driver ends up bound with a NULL tps->irq_domain. Unloading the
module then makes tps65217_remove() call irq_domain_remove() on the
NULL pointer and oops the kernel. On top of that, irq_find_mapping()
may fall back to the default irq domain and dispose of mappings that
belong to other interrupt controllers.

Check the return value and abort the probe on failure so the error
is reported and no inconsistent state is left for removal.

Fixes: 6556bdacf646fcaa ("mfd: tps65217: Add support for IRQs")
Cc: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
 drivers/mfd/tps65217.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index c240fac0ede7..2d04d9e0ae29 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) {
-		tps65217_irq_init(tps, client->irq);
+		ret = tps65217_irq_init(tps, client->irq);
+		if (ret)
+			return ret;
 	} else {
 		int i;
 
-- 
2.53.0

[PATCH v2 2/4] mfd: tps65217: Check return value when masking interrupt sources

[Жамбакиев Радий Рикардинович]

tps65217_irq_init() ignores the error returned by
tps65217_set_bits() when masking all interrupt sources. A failed
register write leaves the driver's software mask out of sync with the
hardware and may result in spurious interrupts.

Check the return value and propagate the error to the caller.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 6556bdacf646fcaa ("mfd: tps65217: Add support for IRQs")
Cc: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
 drivers/mfd/tps65217.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index 2d04d9e0ae29..9a1528456ffc 100644
--- a/drivers/mfd/tps65217.c
+++ b/drivers/mfd/tps65217.c
@@ -155,8 +155,13 @@ static int tps65217_irq_init(struct tps65217 *tps, int irq)
 
 	/* Mask all interrupt sources */
 	tps->irq_mask = TPS65217_INT_MASK;
-	tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
-			  TPS65217_INT_MASK, TPS65217_PROTECT_NONE);
+	ret = tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
+				TPS65217_INT_MASK, TPS65217_PROTECT_NONE);
+	if (ret) {
+		dev_err(tps->dev, "Failed to mask interrupt sources: %d\n",
+			ret);
+		return ret;
+	}
 
 	tps->irq_domain = irq_domain_create_linear(dev_fwnode(tps->dev), TPS65217_NUM_IRQ,
 						   &tps65217_irq_domain_ops, tps);
-- 
2.53.0

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

[Жамбакиев Радий Рикардинович]

If tps65217_probe() fails after the irq_domain has been created, the
domain is never removed. The tps65217 structure is freed by devres,
leaving the globally registered irq_domain with its host_data pointing
to freed memory, which would trigger a use-after-free if the domain is
ever looked up again, and leaks the domain otherwise.

Move the chip revision read ahead of the IRQ initialization so that
child devices are only probed once the chip has been validated, and
add a cleanup helper that disposes the IRQ mappings and removes the
irq_domain. Call it from the devm_request_threaded_irq() error path in
tps65217_irq_init() and from the devm_mfd_add_devices() error path in
tps65217_probe().

Fixes: 6556bdacf646fcaa ("mfd: tps65217: Add support for IRQs")
Cc: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
 drivers/mfd/tps65217.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index 9a1528456ffc..d535d140c2e9 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);
+		if (virq)
+			irq_dispose_mapping(virq);
+	}
+
+	irq_domain_remove(tps->irq_domain);
+	tps->irq_domain = NULL;
+}
+
 static int tps65217_irq_init(struct tps65217 *tps, int irq)
 {
 	int ret;
@@ -176,6 +194,7 @@ static int tps65217_irq_init(struct tps65217 *tps, int irq)
 	if (ret) {
 		dev_err(tps->dev, "Failed to request IRQ %d: %d\n",
 			irq, ret);
+		tps65217_irq_cleanup(tps);
 		return ret;
 	}
 
@@ -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)
@@ -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);
 		return ret;
 	}
 
-- 
2.53.0

[PATCH v2 4/4] 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: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
 drivers/mfd/tps65217.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index d535d140c2e9..9f4afbaa6524 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);
+
+	tps65217_irq_cleanup(tps);
 }
 
 static const struct i2c_device_id tps65217_id_table[] = {
-- 
2.53.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The `tps65217s` mfd_cell array is a mutable static array globally shared across instances, and its elements are permanently modified during probe.
- [High] Missing zero check on the return value of `irq_find_mapping()` leads to a potential NULL pointer dereference in the interrupt handler.
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race condition in unprotected read-modify-write and password-protected register sequences.
--

--- Patch [1]: [PATCH v2 1/4] mfd: tps65217: Fix NULL pointer dereference on IRQ init failure ---
commit 0c3c0828f7215d1d780ea170098321e2547d6b88
Author: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>

mfd: tps65217: Fix NULL pointer dereference on IRQ init failure
    
tps65217_probe() ignores the return value of tps65217_irq_init(), so
when the irq domain creation fails the probe still completes and the
driver ends up bound with a NULL tps->irq_domain. Unloading the
module then makes tps65217_remove() call irq_domain_remove() on the
NULL pointer and oops the kernel. On top of that, irq_find_mapping()
may fall back to the default irq domain and dispose of mappings that
belong to other interrupt controllers.
    
Check the return value and abort the probe on failure so the error
is reported and no inconsistent state is left for removal.

> 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
> @@ -83,8 +83,8 @@ static struct irq_chip tps65217_irq_chip = {
>  	.irq_enable		= tps65217_irq_enable,
>  	.irq_disable		= tps65217_irq_disable,
>  };
>  
>  static struct mfd_cell tps65217s[] = {

[Severity: High]
This isn't a bug introduced by this patch, but should the tps65217s
mfd_cell array be defined as static const?

Because it is a mutable static array globally shared across instances,
if one PMIC instance is probed without an IRQ, the driver modifies the
global static array in 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;
	}

Could this permanently zero out the resources for all future instances of
the PMIC driver on the system, breaking subsequent instances that do possess
an IRQ?

[ ... ]

> @@ -121,8 +121,8 @@ static irqreturn_t tps65217_irq_thread(int irq, void *data)
>  	}
>  
>  	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
>  		if (status & BIT(i)) {
>  			handle_nested_irq(irq_find_mapping(tps->irq_domain, i));

[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer
dereference?

If a spurious interrupt or an unmapped interrupt triggers,
irq_find_mapping() will return 0.

Would passing 0 directly to handle_nested_irq() lead to a panic?

[ ... ]

> @@ -218,12 +218,12 @@ int tps65217_reg_write(struct tps65217 *tps, unsigned int reg,
>  
>  		return regmap_write(tps->regmap, reg, val);
>  	case TPS65217_PROTECT_L2:
>  		xor_reg_val = reg ^ TPS65217_PASSWORD_REGS_UNLOCK;
>  		ret = regmap_write(tps->regmap, TPS65217_REG_PASSWORD,
>  							xor_reg_val);
>  		if (ret < 0)
>  			return ret;
>  		ret = regmap_write(tps->regmap, reg, val);

[Severity: High]
This isn't a bug introduced by this patch, but does the multi-step password
unlock sequence lack driver-level locking?

If concurrent threads (e.g. from regulator and charger child drivers) write
to a password-protected register, could their I2C transactions interleave
and break the PMIC unlock state machine?

[ ... ]

> @@ -248,8 +248,8 @@ static int tps65217_update_bits(struct tps65217 *tps, unsigned int reg,
>  		unsigned int mask, unsigned int val, unsigned int level)
>  {
>  	int ret;
>  	unsigned int data;
>  
>  	ret = tps65217_reg_read(tps, reg, &data);

[Severity: High]
This is a pre-existing issue, but does this function have a Time-Of-Check
to Time-Of-Use (TOCTOU) race condition?

Because the read and write operations are separate regmap calls without a
lock, could concurrent updates interleave and result in lost register
updates or data corruption?

[ ... ]

> @@ -333,7 +333,9 @@ static int tps65217_probe(struct i2c_client *client)
>  	}
>  
>  	if (client->irq) {
> -		tps65217_irq_init(tps, client->irq);
> +		ret = tps65217_irq_init(tps, client->irq);
> +		if (ret)
> +			return ret;
>  	} else {
>  		int i;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821123712.260443-1-r.zhambakiev@prosoftsystems.ru?part=1

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

[sashiko-bot]

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: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>

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: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

> 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/20260821123712.260443-1-r.zhambakiev@prosoftsystems.ru?part=4

Re: [PATCH v2 2/4] mfd: tps65217: Check return value when masking interrupt sources

[sashiko-bot]

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

Pre-existing issues:
- [High] The global `mfd_cell` array `tps65217s` is not defined as `static const` and is dynamically mutated during probe, breaking subsequent device instances.
- [High] The driver hardcodes `-1` (`PLATFORM_DEVID_NONE`) for cell indexing in `devm_mfd_add_devices()`, causing sysfs device naming collisions.
- [High] Unchecked return value of `irq_find_mapping()` is passed directly to `handle_nested_irq()`, potentially causing a NULL pointer dereference or invalid IRQ execution.
- [High] Read-modify-write operations on the clear-on-read `TPS65217_REG_INT` register silently drop pending hardware interrupts.
- [Medium] The IRQ wake count is unbalanced due to a missing `disable_irq_wake()` call on probe error paths.
--

--- Patch [2]: [PATCH v2 2/4] mfd: tps65217: Check return value when masking interrupt sources ---
commit 07181fdc30db713af1912813b5a67b5c8d55e141
Author: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>

mfd: tps65217: Check return value when masking interrupt sources

This commit modifies tps65217_irq_init() to check the return value of
tps65217_set_bits() when masking interrupt sources. It propagates the error
if the register write fails, preventing the software mask from going out of
sync with the hardware.

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 2d04d9e0ae29c..9a1528456ffc2 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -155,8 +155,13 @@ static int tps65217_irq_init(struct tps65217 *tps, int irq)
>  
>  	/* Mask all interrupt sources */
>  	tps->irq_mask = TPS65217_INT_MASK;
> -	tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> -			  TPS65217_INT_MASK, TPS65217_PROTECT_NONE);
> +	ret = tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> +				TPS65217_INT_MASK, TPS65217_PROTECT_NONE);
> +	if (ret) {
> +		dev_err(tps->dev, "Failed to mask interrupt sources: %d\n",
> +			ret);
> +		return ret;
> +	}
>  
>  	tps->irq_domain = irq_domain_create_linear(dev_fwnode(tps->dev), TPS65217_NUM_IRQ,
>  						   &tps65217_irq_domain_ops, tps);

[ ... ]

> static void tps65217_irq_sync_unlock(struct irq_data *data)
> {
> 	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
> 	int ret;
> 
> 	ret = tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> 				tps->irq_mask, TPS65217_PROTECT_NONE);

[Severity: High]
This is a pre-existing issue, but does performing a read-modify-write via
tps65217_set_bits() on the clear-on-read TPS65217_REG_INT register silently
drop pending hardware interrupts? 

[ ... ]

> static struct mfd_cell tps65217s[] = {

[Severity: High]
This isn't a bug introduced by this patch, but does the tps65217s array lack a
const qualifier?

> 	{
> 		.name = "tps65217-pmic",
> 		.of_compatible = "ti,tps65217-pmic",
> 	},

[ ... ]

> static irqreturn_t tps65217_irq_thread(int irq, void *data)
> {

[ ... ]

> 	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
> 		if (status & BIT(i)) {
> 			handle_nested_irq(irq_find_mapping(tps->irq_domain, i));

[Severity: High]
This is a pre-existing issue, but does this code unconditionally pass the
result of irq_find_mapping() to handle_nested_irq() without verifying it is
non-zero?

Since irq_find_mapping() returns 0 if an interrupt is unmapped, could passing
0 to handle_nested_irq() lead to a NULL pointer dereference in irq_to_desc(0)
or falsely trigger the system timer interrupt handler if an unmapped bit is
set?

> 			handled = true;
> 		}
> 	}

[ ... ]

> static int tps65217_probe(struct i2c_client *client)
> {

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but does modifying
tps65217s[i].num_resources permanently corrupt this shared global template?

If a TPS65217 device is probed without an IRQ, wouldn't subsequent instances
or deferred probes observe the corrupted zeroed resources?

> 	}
> 
> 	ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
> 				   ARRAY_SIZE(tps65217s), NULL, 0,
> 				   tps->irq_domain);

[Severity: High]
This is a pre-existing issue, but doesn't hardcoding -1 (PLATFORM_DEVID_NONE)
for cell indexing prevent the MFD core from appending unique instance IDs to
child devices?

Should PLATFORM_DEVID_AUTO be used instead to avoid duplicate sysfs device
naming collisions if multiple chips are present?

> 	if (ret < 0) {
> 		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> 		return ret;

[Severity: Medium]
This isn't a bug introduced by this patch, but if tps65217_irq_init() succeeds
and enables IRQ wake, does this error path leave the IRQ wake count unbalanced
since it misses a disable_irq_wake() call before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821123712.260443-1-r.zhambakiev@prosoftsystems.ru?part=2

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

[sashiko-bot]

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: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>
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: stable@vger.kernel.org

> 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/20260821123712.260443-1-r.zhambakiev@prosoftsystems.ru?part=3

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

[Lee Jones]

On Fri, 21 Aug 2026, sashiko-bot@kernel.org wrote:

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

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> 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: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>
> 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: stable@vger.kernel.org
> 
> > 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/20260821123712.260443-1-r.zhambakiev@prosoftsystems.ru?part=3

-- 
Lee Jones