Re: [PATCH] i3c: renesas: Don't register devices when ENTDAA times out

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] I2C and I3C transfer loops silently ignore timeouts and controller errors, unconditionally reporting success to callers.
- [High] I3C transfers lack FIFO depth validation and pointer accumulation, leading to immediate TX overflow and RX buffer corruption for transfers > 64 bytes.
- [High] Hardware controller is not aborted or reset after a software timeout, leading to permanent state machine desynchronization and downstream heap overflow.
- [High] Missing bounds check in RX ISRs allows unbounded data reads from the hardware FIFO, leading to kernel heap buffer overflows.
--

--- Patch [1]: [PATCH] i3c: renesas: Don't register devices when ENTDAA times out ---
commit bec52da958c86846802998b536f26cd9996c3167
Author: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Subject: i3c: renesas: Don't register devices when ENTDAA times out
    
renesas_i3c_daa() derives the number of newly assigned dynamic addresses
from cmd->rx_count, which the response ISR sets to the number of address
slots ENTDAA left unassigned. It starts out as zero, which already means
"every address was assigned", so a timed out transfer leaves that value
in place and it gets used as a result.

Start from maxdevs instead: no address is assigned before ENTDAA runs,
and the existing rx_count >= maxdevs check then reports an empty bus.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index ff1a243a802ed..28c0927a0179c 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -684,7 +684,7 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  
>  	init_completion(&xfer->comp);
>  	cmd = xfer->cmds;
> -	cmd->rx_count = 0;
> +	cmd->rx_count = i3c->maxdevs;
>  
>  	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
>  	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);

[ ... ]

> static unsigned long renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> {
> 	unsigned long time_left;
> 
> 	renesas_i3c_enqueue_xfer(i3c, xfer);
> 
> 	time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
> 	if (!time_left)
> 		renesas_i3c_dequeue_xfer(i3c, xfer);
> 
> 	return time_left;
> }

[Severity: High]
This isn't a bug introduced by this patch, but does this code abandon the
software transfer on timeout without stopping the hardware controller?

If a timeout occurs, interrupts are masked but no hardware abort or reset
commands are issued. Could this lead to permanent state machine
desynchronization and downstream heap overflows when stale hardware responses
are processed against newly queued transfer buffers?

[ ... ]

> static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_xfers,
> 				 int i3c_nxfers, enum i3c_xfer_mode mode)
> {
...
> 		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
> 			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
> 			if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
> 				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> 		}
> 
> 		time_left = renesas_i3c_wait_xfer(i3c, xfer);
> 		if (!time_left)
> 			xfer_failed = true;
> 	}
> 
> 	if (xfer_failed)
> 		renesas_i3c_irqs_mask_and_clear(i3c);
> 
> 	return 0;
> }

[Severity: High]
This is a pre-existing issue, but does i3c_writel_fifo() unconditionally write
cmd->len bytes into the FIFO from the start of cmd->tx_buf without checking
the hardware FIFO depth capacity?

Could this cause immediate hardware FIFO overflow for transfers larger than 64
bytes?

Additionally, does this function completely ignore timeouts and errors? It
unconditionally returns 0 (success) even when a timeout occurs and xfer_failed
is set to true. Could this cause drivers to receive garbage data on reads or
incorrectly assume their writes succeeded?

[ ... ]

> static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
> 					struct i2c_msg *i2c_xfers,
> 					int i2c_nxfers)
> {
...
> 		time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> 		if (!time_left)
> 			xfer_failed = true;
> 
> 		if (cmd->err)
> 			break;
...
> 	renesas_i3c_dequeue_xfer(i3c, xfer);
> 
> 	if (xfer_failed)
> 		renesas_i3c_irqs_mask_and_clear(i3c);
> 
> 	return cmd->err;
> }

[Severity: High]
This isn't a bug introduced by this patch, but does this function also ignore
timeouts?

When wait_for_completion_timeout expires, cmd->err is not populated with
-ETIMEDOUT, causing the function to silently return 0.

[ ... ]

> static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
> {
...
> 		} else {
> 			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
> 		}
> 	}
> 
> 	return IRQ_HANDLED;
> }

[Severity: High]
This is a pre-existing issue, but does this path unconditionally copy the
entire cmd->len to the hardware FIFO from the start of cmd->tx_buf on every
interrupt, without pointer advancement or FIFO depth checks?

[ ... ]

> static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
> {
...
> 		case I3C_INTERNAL_STATE_CONTROLLER_READ:
> 		case I3C_INTERNAL_STATE_CONTROLLER_COMMAND_READ:
> 			if (!cmd->err) {
> 				u32 rx_count = min(cmd->rx_count, data_len);
> 
> 				bytes_remaining = data_len - rx_count;
> 				if (bytes_remaining)
> 					i3c_readl_fifo(i3c->regs + NTDTBP0,
> 						       cmd->rx_buf + rx_count,
> 						       bytes_remaining);
> 				cmd->rx_count = data_len;
> 			}
> 			renesas_clear_bit(i3c->regs, NTIE, NTIE_RDBFIE0);
> 			break;

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing bounds check
here?

The driver calculates bytes_remaining based solely on data_len from the
hardware response descriptor (NRSPQP_DATA_LEN), without clamping it against
the software-allocated buffer size (cmd->len). If a stale response from a prior
desynchronized transfer contains a large payload length, could this overflow
cmd->rx_buf into adjacent kernel heap memory?

[ ... ]

> static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
> {
...
> 		} else {
> 			read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
> 			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
> 			cmd->rx_count = read_bytes;
> 		}
> 
> 		/* Clear the Read Buffer Full status flag. */
> 		renesas_clear_bit(i3c->regs, NTST, NTST_RDBFF0);
> 	}
> 
> 	return IRQ_HANDLED;
> }

[Severity: High]
This is a pre-existing issue, but does this function overwrite cmd->rx_buf
at offset 0 instead of accumulating data? 

Also, it appears to read into the buffer based purely on the hardware-reported
length without bounding against cmd->len. Could this also lead to kernel heap
buffer overflows?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731070150.2519825-1-tommaso.merciai.xr@bp.renesas.com?part=1

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