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

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: Жамбакиев Радий Рикардинович <[email protected]>

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/[email protected]?part=1