* [PATCH] i3c: master: svc: bound IBI payload to the requested max_payload_len
@ 2026-06-23 15:51 ` Maoyi Xie
0 siblings, 0 replies; 5+ messages in thread
From: Maoyi Xie @ 2026-06-23 15:51 UTC (permalink / raw)
To: Miquel Raynal, Frank Li
Cc: Alexandre Belloni, Kaixuan Li, linux-i3c, linux-kernel, stable
svc_i3c_master_handle_ibi() reads the IBI payload from the RX FIFO into
the IBI slot. The loop is bounded by the hardware FIFO size
(SVC_I3C_FIFO_SIZE), not by the slot size.
slot->data points into the IBI pool, which i3c_generic_ibi_alloc_pool()
sizes at max_payload_len per slot. svc_i3c_master_request_ibi() only
rejects a max_payload_len larger than SVC_I3C_FIFO_SIZE, so a driver can
request a smaller one. mctp-i3c requests 1. Each readsb() then copies the
controller RXCOUNT bytes (up to 31) with no check against the slot size.
A device that sends more bytes than the slot holds writes past
slot->data, an out-of-bounds write into the IBI pool.
Bound the loop by dev->ibi->max_payload_len and clamp each read to the
space left in the slot, the same way dw-i3c does.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: stable@vger.kernel.org
Co-developed-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
Signed-off-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
drivers/i3c/master/svc-i3c-master.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e2d99a3ac07d..7420bfbdd259 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -465,9 +465,11 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
buf = slot->data;
while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
- slot->len < SVC_I3C_FIFO_SIZE) {
+ slot->len < dev->ibi->max_payload_len) {
mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
+ count = min_t(unsigned int, count,
+ dev->ibi->max_payload_len - slot->len);
readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
slot->len += count;
buf += count;
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] i3c: master: svc: bound IBI payload to the requested max_payload_len
2026-06-23 15:51 ` Maoyi Xie
(?)
@ 2026-06-23 16:06 ` sashiko-bot
-1 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-06-23 16:06 UTC (permalink / raw)
To: Maoyi Xie; +Cc: Frank.Li, linux-i3c
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The patch limits the IBI payload read loop to `max_payload_len` but fails to flush remaining excess bytes from the hardware RX FIFO, causing permanent FIFO desynchronization and data corruption.
Pre-existing issues:
- [High] Inconsistent use of stream accessors (`readsb`/`writesb`) and register accessors (`readl`/`writel`) for the same FIFO addresses causes severe data corruption on big-endian architectures.
- [High] Receiving an IBI from a device before its driver has called `i3c_device_request_ibi()` causes unconditional NULL pointer dereferences when attempting to access IBI structures.
--
commit c4892fd6b08fdfd54b181eeaaa9518cb8c197363
Author: Maoyi Xie <maoyixie.tju@gmail.com>
Subject: i3c: master: svc: bound IBI payload to the requested max_payload_len
This commit bounds the IBI payload read loop in
svc_i3c_master_handle_ibi() to max_payload_len to prevent out-of-bounds
writes into the IBI pool. It clamps each read to the remaining space left
in the slot.
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -455,9 +455,11 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> buf = slot->data;
>
> while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
> - slot->len < SVC_I3C_FIFO_SIZE) {
> + slot->len < dev->ibi->max_payload_len) {
[Severity: High]
This is a pre-existing issue, but does this code assume dev->ibi is always
allocated?
If a device initiates an IBI before its driver has called
i3c_device_request_ibi(), dev->ibi and data->ibi_pool remain unallocated.
Looking at svc_i3c_master_ibi_isr(), it appears to accept all IBIs based on a
global mask rather than checking if dev->ibi is initialized:
svc_i3c_master_ibi_isr() {
if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) {
svc_i3c_master_nack_ibi(master);
} else {
...
svc_i3c_master_handle_ibi(master, dev);
}
}
When it calls svc_i3c_master_handle_ibi(), the code dereferences dev->ibi
here, and also passes data->ibi_pool into i3c_generic_ibi_get_free_slot().
Could this trigger a null pointer dereference if the device sends an early
IBI?
> mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
> count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
> + count = min_t(unsigned int, count,
> + dev->ibi->max_payload_len - slot->len);
> readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
[Severity: High]
This isn't a bug introduced by this patch, but is there an endianness mismatch
across the driver when accessing this FIFO?
This code correctly uses the stream accessor readsb() to read from
SVC_I3C_MRDATAB without byteswapping. However, looking at
svc_i3c_master_read(), the identical FIFO address is read using the 32-bit
register accessor readl():
svc_i3c_master_read() {
...
dst[i] = readl(master->regs + SVC_I3C_MRDATAB);
...
}
On big-endian systems, readl() swaps the 32-bit word, which might place the
valid byte into the highest byte before being truncated to 0x00 when assigned
to a u8 array.
Similarly, svc_i3c_master_do_xfer() mixes stream and register accessors for
the same transmission:
svc_i3c_master_do_xfer() {
...
writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
writel(out[len - 1] | end, master->regs + SVC_I3C_MWDATAB);
...
}
Could this mix of accessors cause data corruption on big-endian architectures?
> slot->len += count;
> buf += count;
> }
[Severity: High]
If an I3C device sends an IBI payload larger than dev->ibi->max_payload_len,
the loop terminates early. Will this leave unread bytes trapped in the
hardware RX FIFO?
Since the transfer was already polled as complete before the loop, returning
here without draining the remaining bytes from SVC_I3C_MRDATAB or calling
writel(SVC_I3C_MDATACTRL_FLUSHRB, ...) might cause subsequent transfers to
consume these residual bytes.
Could this lead to FIFO desynchronization if a device sends an unexpected
number of bytes?
--
Sashiko AI review · https://sashiko.dev/#/patchset/178222990006.2767135.12462569914183698733@maoyixie.com?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: svc: bound IBI payload to the requested max_payload_len
2026-06-23 15:51 ` Maoyi Xie
@ 2026-06-23 16:53 ` Frank Li
-1 siblings, 0 replies; 5+ messages in thread
From: Frank Li @ 2026-06-23 16:53 UTC (permalink / raw)
To: Maoyi Xie
Cc: Miquel Raynal, Frank Li, Alexandre Belloni, Kaixuan Li, linux-i3c,
linux-kernel, stable
On Tue, Jun 23, 2026 at 11:51:40PM +0800, Maoyi Xie wrote:
> svc_i3c_master_handle_ibi() reads the IBI payload from the RX FIFO into
> the IBI slot. The loop is bounded by the hardware FIFO size
> (SVC_I3C_FIFO_SIZE), not by the slot size.
>
> slot->data points into the IBI pool, which i3c_generic_ibi_alloc_pool()
> sizes at max_payload_len per slot. svc_i3c_master_request_ibi() only
> rejects a max_payload_len larger than SVC_I3C_FIFO_SIZE, so a driver can
> request a smaller one. mctp-i3c requests 1. Each readsb() then copies the
> controller RXCOUNT bytes (up to 31) with no check against the slot size.
> A device that sends more bytes than the slot holds writes past
> slot->data, an out-of-bounds write into the IBI pool.
>
> Bound the loop by dev->ibi->max_payload_len and clamp each read to the
> space left in the slot, the same way dw-i3c does.
>
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: stable@vger.kernel.org
> Co-developed-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
> Signed-off-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
> Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
> ---
> drivers/i3c/master/svc-i3c-master.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e2d99a3ac07d..7420bfbdd259 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -465,9 +465,11 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> buf = slot->data;
>
> while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
> - slot->len < SVC_I3C_FIFO_SIZE) {
> + slot->len < dev->ibi->max_payload_len) {
> mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
> count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
> + count = min_t(unsigned int, count,
> + dev->ibi->max_payload_len - slot->len);
now needn't min_t, only min() should be good
see:
https://lore.kernel.org/all/20251119224140.8616-1-david.laight.linux@gmail.com/
Frank
> readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
> slot->len += count;
> buf += count;
--
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: svc: bound IBI payload to the requested max_payload_len
@ 2026-06-23 16:53 ` Frank Li
0 siblings, 0 replies; 5+ messages in thread
From: Frank Li @ 2026-06-23 16:53 UTC (permalink / raw)
To: Maoyi Xie
Cc: Miquel Raynal, Frank Li, Alexandre Belloni, Kaixuan Li, linux-i3c,
linux-kernel, stable
On Tue, Jun 23, 2026 at 11:51:40PM +0800, Maoyi Xie wrote:
> svc_i3c_master_handle_ibi() reads the IBI payload from the RX FIFO into
> the IBI slot. The loop is bounded by the hardware FIFO size
> (SVC_I3C_FIFO_SIZE), not by the slot size.
>
> slot->data points into the IBI pool, which i3c_generic_ibi_alloc_pool()
> sizes at max_payload_len per slot. svc_i3c_master_request_ibi() only
> rejects a max_payload_len larger than SVC_I3C_FIFO_SIZE, so a driver can
> request a smaller one. mctp-i3c requests 1. Each readsb() then copies the
> controller RXCOUNT bytes (up to 31) with no check against the slot size.
> A device that sends more bytes than the slot holds writes past
> slot->data, an out-of-bounds write into the IBI pool.
>
> Bound the loop by dev->ibi->max_payload_len and clamp each read to the
> space left in the slot, the same way dw-i3c does.
>
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: stable@vger.kernel.org
> Co-developed-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
> Signed-off-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
> Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
> ---
> drivers/i3c/master/svc-i3c-master.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e2d99a3ac07d..7420bfbdd259 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -465,9 +465,11 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> buf = slot->data;
>
> while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
> - slot->len < SVC_I3C_FIFO_SIZE) {
> + slot->len < dev->ibi->max_payload_len) {
> mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
> count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
> + count = min_t(unsigned int, count,
> + dev->ibi->max_payload_len - slot->len);
now needn't min_t, only min() should be good
see:
https://lore.kernel.org/all/20251119224140.8616-1-david.laight.linux@gmail.com/
Frank
> readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
> slot->len += count;
> buf += count;
^ permalink raw reply [flat|nested] 5+ messages in thread