[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