Re: [PATCH v3 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds checks and integer underflow in RX interrupt handlers lead to massive out-of-bounds heap writes.
- [High] I3C and I2C transfer loops ignore hardware errors and timeouts, silently returning success and risking state corruption.
--

commit 1be8528941940333e90705770f0a977f95d9b3d5
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Follow a unified pattern for transfer and command initialization

Follow a unified pattern for transfer and command initialization across
the driver. This keeps the code cleaner and easier to follow. Also, in
some cases the I3C device was enabled before the transfer data structure
was even allocated.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -939,15 +939,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
>  	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
>  	int i;
>  
> -	/* Enable I3C bus. */
> -	renesas_i3c_bus_enable(m, true);
> -
>  	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
>  	if (!xfer)
>  		return -ENOMEM;
>  
>  	init_completion(&xfer->comp);
>  
> +	/* Enable I3C bus. */
> +	renesas_i3c_bus_enable(m, true);
> +
>  	for (i = 0; i < i3c_nxfers; i++) {
>  		struct renesas_i3c_cmd *cmd = xfer->cmds;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the transfer
initialization here, I noticed an issue in the loop below.

Does the loop ignore hardware errors and timeouts? It looks like the hardware
error status in xfer->ret is not checked, and the function unconditionally
returns 0:

    time_left = renesas_i3c_wait_xfer(i3c, xfer);
    if (!time_left)
        abort_xfer = true;
}

if (abort_xfer)
    renesas_i3c_abort_xfer(i3c);

return 0;

Also, on timeout, it sets abort_xfer = true but doesn't break out of the
loop, which means it reuses the active xfer struct for the next transfer
while the hardware hasn't been aborted yet. Can this lead to state
corruption or information leaks?

[ ... ]
> @@ -1073,12 +1073,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	renesas_i3c_bus_enable(m, false);
> -
>  	init_completion(&xfer->comp);
>  	xfer->is_i2c_xfer = true;
>  	cmd = xfer->cmds;
>  
> +	renesas_i3c_bus_enable(m, false);
> +
>  	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
>  		cmd->err = -EBUSY;
>  		return cmd->err;

[Severity: High]
This is also a pre-existing issue, but I see a similar pattern in
renesas_i3c_i2c_xfers() where it might fail to break out of the loop
when a timeout occurs:

    time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
    if (!time_left)
        abort_xfer = true;

    if (cmd->err)
        break;

Since cmd->err remains 0 on timeout, the loop continues to the next message
and eventually returns 0 instead of propagating the timeout. Should the loop
break when abort_xfer is set to true?

[ ... ]

[Severity: Critical]
This is a pre-existing issue not introduced by this patch, but while
reviewing the driver I noticed potential out-of-bounds writes in the
RX interrupt handlers.

In renesas_i3c_rx_isr(), is there a missing bounds check against cmd->len?
The data seems to be read directly into cmd->rx_buf based on the FIFO fill
level without validating it against the allocated length:

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

It looks like this also overwrites the start of cmd->rx_buf and overwrites
cmd->rx_count instead of appending or incrementing.

Additionally, in renesas_i3c_resp_isr(), does the subtraction risk integer
underflow?

    if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
        bytes_remaining = data_len - cmd->rx_count;

    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);

If data_len from the response descriptor is smaller than cmd->rx_count, this
subtraction could underflow. When passed to i3c_readl_fifo(), the value
could become a negative integer, which is implicitly cast to a massive
positive size in readsl(). Could this cause a remote heap overflow?

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

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