Re: [PATCH v3 08/17] i3c: renesas: Clean DATBAS register on detach

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] NULL pointer dereferences in slot grouping and unnecessary indirection during resume.
- [High] Undefined behavior from GENMASK when no devices attach during ENTDAA.
- [High] Hardware state corruption due to missing cleanup of DATBAS registers in DAA.
--

commit 0d20150d70b84df8abaeebdb009b4fee9f1d4c9b
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Clean DATBAS register on detach

The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f96848022c45d..195c40956148b 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1047,6 +1047,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
>  
> +	renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are clearing DATBAS
registers here to avoid hardware issues, should we also clean them up in the
error and skip paths of renesas_i3c_daa()?

In renesas_i3c_daa(), we eagerly program DATBAS for all free slots before
initiating ENTDAA:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
		ret = i3c_master_get_free_addr(m, last_addr + 1);
		if (ret < 0)
			return -ENOSPC;

		i3c->addrs[pos].addr = ret;
		last_addr = ret;

		renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
    ...
}

If i3c_master_get_free_addr() fails midway, it returns -ENOSPC without clearing
the previously programmed DATBAS registers. 

Additionally, if ENTDAA assigns fewer devices than available slots, the
remaining unassigned slots are skipped without reverting their DATBAS registers
to 0:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
	for (pos = 0; pos < i3c->maxdevs; pos++) {
		if (!(newdevs & BIT(pos)))
			continue;
    ...
}

Could this leave stale pre-assigned addresses in DATBAS and cause similar bus
collisions or false ACKs?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_group_devs_in_slots()
blindly dereference a NULL pointer if an I2C device is being swapped into an
empty slot?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_group_devs_in_slots() {
    ...
		i3c_addr = i3c->addrs[i2c_pos];
		i3c_dev = i3c_addr.i3c_dev;
    ...
		new_i2c_data = i3c_dev_get_master_data(i3c_dev->dev->desc);
    ...
}

If i2c_pos points to a free slot (e.g., if a device was detached earlier),
i3c_addr.i3c_dev is NULL, and dereferencing i3c_dev->dev will cause an
immediate kernel panic.

Additionally, since i3c_dev is already a struct i3c_dev_desc *, is it necessary
to do the redundant indirection i3c_dev->dev->desc? A similar redundant
indirection happens during resume in renesas_i3c_daa():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
		if (!i3c->free_pos && i3c->resuming) {
			struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev->dev->desc;
    ...
}

Will this oops if the Linux device wrapper (dev) failed to allocate or was
unassigned?


[Severity: High]
This is also a pre-existing issue, but can GENMASK invoke undefined behavior
in renesas_i3c_daa() if no devices attach during ENTDAA?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
	/* Skip attaching if there are failures on the xfer. */
	if (xfer->ret) {
		newdevs = 0;
	} else {
		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
    ...
}

If the hardware executes an ENTDAA command but no new devices are found or
assigned, the response queue returns the remaining unassigned count in data_len.
This sets cmd->rx_count to the requested device count
(i3c->maxdevs - first_i3c_pos).

When this happens, the GENMASK macro is evaluated with high as first_i3c_pos - 1
and low as first_i3c_pos. Since high < low, doesn't this trigger an
out-of-bounds bitwise shift and yield a garbage mask, corrupting the newdevs
mask?

>  	i3c_dev_set_master_data(dev, NULL);
>  	i3c->addrs[data->index].addr = 0;
>  	i3c->addrs[data->index].i3c_dev = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=8

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c