* Re: [PATCH] i3c: master: renesas: reset xfer status before each wait
2026-06-25 3:03 [PATCH] i3c: master: renesas: reset xfer status before each wait Pengpeng Hou
@ 2026-06-25 3:20 ` sashiko-bot
2026-06-25 14:35 ` Frank Li
2026-06-25 14:28 ` Frank Li
2026-07-02 9:06 ` Claudiu Beznea
2 siblings, 1 reply; 5+ messages in thread
From: sashiko-bot @ 2026-06-25 3:20 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: linux-i3c, Frank.Li
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Buffer Overflow and Data Corruption in Transfer Interrupt Handlers.
- [High] Missing Hardware Teardown on Timeout causing Use-After-Free and NULL Pointer Dereference.
- [High] Ignored transfer timeout leads to Hardware State Desync and silent failures.
- [High] Unvalidated hardware response length causes Integer Underflow and Undefined Behavior in GENMASK.
--
commit 1827d9b795e70cd1f909f688b702f06c732c89e1
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
i3c: master: renesas: reset xfer status before each wait
renesas_i3c_wait_xfer() reinitializes the completion before queuing a
transfer, but it leaves xfer->ret unchanged. After one transfer completes
successfully, a later timeout can therefore reuse the old zero status.
Reset xfer->ret to -ETIMEDOUT before each wait, return the transfer
status from the wait helper, and propagate it from DAA, CCC and private
I3C transfers.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..7f123cd72cdf3 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -433,15 +433,18 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
[ ... ]
> -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> +static int renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> {
> unsigned long time_left;
>
> + xfer->ret = -ETIMEDOUT;
> 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 xfer->ret;
> }
[Severity: High]
This isn't a bug introduced by this patch, but does this timeout path safely
abort the active hardware operation?
If a timeout occurs, the driver removes the transfer from the queue via
renesas_i3c_dequeue_xfer() but does not appear to halt the hardware DMA or
interrupt execution.
If the caller then frees the xfer struct, and the hardware later completes the
operation, could the delayed interrupt in renesas_i3c_resp_isr() dereference
a freed or unrelated transfer?
drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
xfer = i3c->xferqueue.cur;
cmd = xfer->cmds;
...
}
[ ... ]
> @@ -687,7 +690,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
> NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> + if (ret)
> + return ret;
>
> newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
[Severity: High]
This is a pre-existing issue, but does this calculation handle the case where
no devices are attached?
If no devices respond during Dynamic Address Assignment, cmd->rx_count will
equal i3c->maxdevs. This would cause the subtraction to evaluate to -1,
leading to undefined behavior in the GENMASK macro.
Could this result in newdevs evaluating to all 1s and incorrectly registering
empty slots as real devices?
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but does renesas_i3c_i2c_xfers()
properly handle transfer timeouts?
When wait_for_completion_timeout() times out, it does not set cmd->err. The
loop then checks if (cmd->err) and proceeds to the next message:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
...
wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
if (cmd->err)
break;
...
}
Could this ignore the timeout, incorrectly return success, and allow a delayed
hardware operation to corrupt the buffer of the subsequent message?
[Severity: Critical]
This is a pre-existing issue, but do the interrupt handlers correctly manage
buffer lengths and offsets for transfers?
In renesas_i3c_rx_isr(), the byte count is rounded up to the nearest 32-bit
word, which can write past the actual buffer length for unaligned transfers:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr() {
...
read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
...
}
Additionally, the transmit ISR writes the entire transfer length without tracking
bytes already sent, which might overflow the hardware FIFO:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_tx_isr() {
...
i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
...
}
Finally, the response ISR reads the remaining bytes into the start of the
buffer, potentially overwriting data previously received by the rx_isr:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
bytes_remaining = data_len - cmd->rx_count;
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
...
}
Could this lead to memory corruption and out-of-bounds writes?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625030357.87402-1-pengpeng@iscas.ac.cn?part=1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] i3c: master: renesas: reset xfer status before each wait
2026-06-25 3:20 ` sashiko-bot
@ 2026-06-25 14:35 ` Frank Li
0 siblings, 0 replies; 5+ messages in thread
From: Frank Li @ 2026-06-25 14:35 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Pengpeng Hou, linux-i3c, Frank.Li
On Thu, Jun 25, 2026 at 03:20:58AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Buffer Overflow and Data Corruption in Transfer Interrupt Handlers.
> - [High] Missing Hardware Teardown on Timeout causing Use-After-Free and NULL Pointer Dereference.
> - [High] Ignored transfer timeout leads to Hardware State Desync and silent failures.
> - [High] Unvalidated hardware response length causes Integer Underflow and Undefined Behavior in GENMASK.
> --
>
> commit 1827d9b795e70cd1f909f688b702f06c732c89e1
> Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
>
> i3c: master: renesas: reset xfer status before each wait
>
> renesas_i3c_wait_xfer() reinitializes the completion before queuing a
> transfer, but it leaves xfer->ret unchanged. After one transfer completes
> successfully, a later timeout can therefore reuse the old zero status.
>
> Reset xfer->ret to -ETIMEDOUT before each wait, return the transfer
> status from the wait helper, and propagate it from DAA, CCC and private
> I3C transfers.
>
> > diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> > index f39c449922ca2..7f123cd72cdf3 100644
> > --- a/drivers/i3c/master/renesas-i3c.c
> > +++ b/drivers/i3c/master/renesas-i3c.c
> > @@ -433,15 +433,18 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
> [ ... ]
> > -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> > +static int renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> > {
> > unsigned long time_left;
> >
> > + xfer->ret = -ETIMEDOUT;
> > 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 xfer->ret;
> > }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this timeout path safely
> abort the active hardware operation?
>
> If a timeout occurs, the driver removes the transfer from the queue via
> renesas_i3c_dequeue_xfer() but does not appear to halt the hardware DMA or
> interrupt execution.
>
> If the caller then frees the xfer struct, and the hardware later completes the
> operation, could the delayed interrupt in renesas_i3c_resp_isr() dereference
> a freed or unrelated transfer?
This is one problem, although it is less possibility. suggest fix later,
you may reduce timeout value and add some debug message isr, this problem
should be triggered.
Frank
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
> ...
> xfer = i3c->xferqueue.cur;
> cmd = xfer->cmds;
> ...
> }
>
> [ ... ]
> > @@ -687,7 +690,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> > NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
> > NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
> >
> > - renesas_i3c_wait_xfer(i3c, xfer);
> > + ret = renesas_i3c_wait_xfer(i3c, xfer);
> > + if (ret)
> > + return ret;
> >
> > newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
>
> [Severity: High]
> This is a pre-existing issue, but does this calculation handle the case where
> no devices are attached?
>
> If no devices respond during Dynamic Address Assignment, cmd->rx_count will
> equal i3c->maxdevs. This would cause the subtraction to evaluate to -1,
> leading to undefined behavior in the GENMASK macro.
>
> Could this result in newdevs evaluating to all 1s and incorrectly registering
> empty slots as real devices?
>
> [ ... ]
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does renesas_i3c_i2c_xfers()
> properly handle transfer timeouts?
>
> When wait_for_completion_timeout() times out, it does not set cmd->err. The
> loop then checks if (cmd->err) and proceeds to the next message:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
> ...
> wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
>
> if (cmd->err)
> break;
> ...
> }
>
> Could this ignore the timeout, incorrectly return success, and allow a delayed
> hardware operation to corrupt the buffer of the subsequent message?
>
> [Severity: Critical]
> This is a pre-existing issue, but do the interrupt handlers correctly manage
> buffer lengths and offsets for transfers?
>
> In renesas_i3c_rx_isr(), the byte count is rounded up to the nearest 32-bit
> word, which can write past the actual buffer length for unaligned transfers:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr() {
> ...
> read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
> i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
> ...
> }
>
> Additionally, the transmit ISR writes the entire transfer length without tracking
> bytes already sent, which might overflow the hardware FIFO:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_tx_isr() {
> ...
> i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
> ...
> }
>
> Finally, the response ISR reads the remaining bytes into the start of the
> buffer, potentially overwriting data previously received by the rx_isr:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
> ...
> bytes_remaining = data_len - cmd->rx_count;
> i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
> ...
> }
>
> Could this lead to memory corruption and out-of-bounds writes?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260625030357.87402-1-pengpeng@iscas.ac.cn?part=1
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i3c: master: renesas: reset xfer status before each wait
2026-06-25 3:03 [PATCH] i3c: master: renesas: reset xfer status before each wait Pengpeng Hou
2026-06-25 3:20 ` sashiko-bot
@ 2026-06-25 14:28 ` Frank Li
2026-07-02 9:06 ` Claudiu Beznea
2 siblings, 0 replies; 5+ messages in thread
From: Frank Li @ 2026-06-25 14:28 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Wolfram Sang, Tommaso Merciai, Alexandre Belloni, Frank Li,
linux-i3c, linux-kernel
On Thu, Jun 25, 2026 at 11:03:57AM +0800, Pengpeng Hou wrote:
subject: reset xfer ret value before each wait
Frank
> renesas_i3c_wait_xfer() reinitializes the completion before queuing a
> transfer, but it leaves xfer->ret unchanged. After one transfer completes
> successfully, a later timeout can therefore reuse the old zero status.
>
> Reset xfer->ret to -ETIMEDOUT before each wait, return the transfer
> status from the wait helper, and propagate it from DAA, CCC and private
> I3C transfers.
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> drivers/i3c/master/renesas-i3c.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca..7f123cd72cdf 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -433,15 +433,18 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
> }
> }
>
> -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> +static int renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> {
> unsigned long time_left;
>
> + xfer->ret = -ETIMEDOUT;
> 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 xfer->ret;
> }
>
> static void renesas_i3c_set_prts(struct renesas_i3c *i3c, u32 val)
> @@ -687,7 +690,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
> NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> + if (ret)
> + return ret;
>
> newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> newdevs &= ~olddevs;
> @@ -800,9 +805,7 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
> }
> }
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> -
> - ret = xfer->ret;
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> if (ret)
> ccc->err = I3C_ERROR_M2;
>
> @@ -815,7 +818,7 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
> struct i3c_master_controller *m = i3c_dev_get_master(dev);
> struct renesas_i3c *i3c = to_renesas_i3c(m);
> struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> - int i;
> + int i, ret;
>
> /* Enable I3C bus. */
> renesas_i3c_bus_enable(m, true);
> @@ -854,7 +857,9 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
> renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> }
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> + if (ret)
> + return ret;
> }
>
> return 0;
> --
> 2.50.1 (Apple Git-155)
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] i3c: master: renesas: reset xfer status before each wait
2026-06-25 3:03 [PATCH] i3c: master: renesas: reset xfer status before each wait Pengpeng Hou
2026-06-25 3:20 ` sashiko-bot
2026-06-25 14:28 ` Frank Li
@ 2026-07-02 9:06 ` Claudiu Beznea
2 siblings, 0 replies; 5+ messages in thread
From: Claudiu Beznea @ 2026-07-02 9:06 UTC (permalink / raw)
To: Pengpeng Hou, Wolfram Sang, Tommaso Merciai, Alexandre Belloni
Cc: Frank Li, linux-i3c, linux-kernel
Hi, Pengpeng,
On 6/25/26 06:03, Pengpeng Hou wrote:
> renesas_i3c_wait_xfer() reinitializes the completion before queuing a
> transfer, but it leaves xfer->ret unchanged.
In the current code the xfer->ret is initialized with -ETIMEDOUT when it is
allocated in renesas_i3c_alloc_xfer().
> After one transfer completes
> successfully, a later timeout can therefore reuse the old zero status.
The patch here took this into account:
https://lore.kernel.org/all/20260612160458.3102106-17-claudiu.beznea@kernel.org/
Please check the places where renesas_i3c_irqs_mask_and_clear() is called.
>
> Reset xfer->ret to -ETIMEDOUT before each wait, return the transfer
> status from the wait helper, and propagate it from DAA, CCC and private
> I3C transfers.
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> drivers/i3c/master/renesas-i3c.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca..7f123cd72cdf 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -433,15 +433,18 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
> }
> }
>
> -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> +static int renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> {
> unsigned long time_left;
>
> + xfer->ret = -ETIMEDOUT;
> 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 xfer->ret;
In some DAA cases this might be a negative number but we could still consider it OK.
> }
>
> static void renesas_i3c_set_prts(struct renesas_i3c *i3c, u32 val)
> @@ -687,7 +690,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
> NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> + if (ret)
> + return ret;
Have you tested this? My previous experiments around this code tell me this
might not be OK for all the cases.
Thank you,
Claudiu
>
> newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> newdevs &= ~olddevs;
> @@ -800,9 +805,7 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
> }
> }
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> -
> - ret = xfer->ret;
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> if (ret)
> ccc->err = I3C_ERROR_M2;
>
> @@ -815,7 +818,7 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
> struct i3c_master_controller *m = i3c_dev_get_master(dev);
> struct renesas_i3c *i3c = to_renesas_i3c(m);
> struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> - int i;
> + int i, ret;
>
> /* Enable I3C bus. */
> renesas_i3c_bus_enable(m, true);
> @@ -854,7 +857,9 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
> renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> }
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + ret = renesas_i3c_wait_xfer(i3c, xfer);
> + if (ret)
> + return ret;
> }
>
> return 0;
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread