[PATCH v2] i3c: master: svc: bound IBI payload to the requested max_payload_len

[PATCH v2] i3c: master: svc: bound IBI payload to the requested max_payload_len

[Maoyi Xie]

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. A device can still send
more than the requested payload. Flush the leftover bytes from the RX FIFO
so they do not leak into the next transfer.

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>
---
v2:
- use min() instead of min_t(), the types already match (Frank Li)
- flush the leftover RX FIFO bytes after the bounded read, so an
  oversized IBI does not desync the next transfer (Sashiko AI review)

 drivers/i3c/master/svc-i3c-master.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e2d99a3ac07d..4eb54f9ee2cd 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -465,14 +465,22 @@ 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(count, dev->ibi->max_payload_len - slot->len);
 		readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
 		slot->len += count;
 		buf += count;
 	}
 
+	/*
+	 * The device may have sent more than the requested payload. Drop the
+	 * extra bytes so they do not leak into the next transfer.
+	 */
+	if (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)))
+		writel(SVC_I3C_MDATACTRL_FLUSHRB, master->regs + SVC_I3C_MDATACTRL);
+
 	master->ibi.tbq_slot = slot;
 
 	return 0;

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

[PATCH v2] i3c: master: svc: bound IBI payload to the requested max_payload_len

[Maoyi Xie]

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. A device can still send
more than the requested payload. Flush the leftover bytes from the RX FIFO
so they do not leak into the next transfer.

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>
---
v2:
- use min() instead of min_t(), the types already match (Frank Li)
- flush the leftover RX FIFO bytes after the bounded read, so an
  oversized IBI does not desync the next transfer (Sashiko AI review)

 drivers/i3c/master/svc-i3c-master.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e2d99a3ac07d..4eb54f9ee2cd 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -465,14 +465,22 @@ 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(count, dev->ibi->max_payload_len - slot->len);
 		readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
 		slot->len += count;
 		buf += count;
 	}
 
+	/*
+	 * The device may have sent more than the requested payload. Drop the
+	 * extra bytes so they do not leak into the next transfer.
+	 */
+	if (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)))
+		writel(SVC_I3C_MDATACTRL_FLUSHRB, master->regs + SVC_I3C_MDATACTRL);
+
 	master->ibi.tbq_slot = slot;
 
 	return 0;

Re: [PATCH v2] i3c: master: svc: bound IBI payload to the requested max_payload_len

[sashiko-bot]

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

Pre-existing issues:
- [High] Spurious IBIs from devices without an active IBI request cause a Use-After-Free and NULL pointer dereference.
- [High] The driver mixes stream accessors (`readsb`/`writesb`) and register accessors (`readl`/`writel`) for the same data FIFOs, leading to data corruption on big-endian architectures.
- [High] Missing RX FIFO flush on the `COMPLETE` polling timeout error path leaves stale bytes to corrupt the next transfer.
- [High] IBI payloads larger than the controller's FIFO size cause a bus stall and deadlock.
--

commit 49c308fac8d6e4e4b86b2881d06b5794c21d6f51
Author: Maoyi Xie <maoyixie.tju@gmail.com>

i3c: master: svc: bound IBI payload to the requested max_payload_len

This patch modifies the IBI payload read loop to bound it by the requested
max_payload_len rather than the hardware FIFO size. This prevents out-of-bounds
writes into the IBI pool when a device sends more data than expected. It also
ensures any leftover bytes are flushed from the RX FIFO so they do not leak
into the next transfer.

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 93805df8a9406..71f1fc6933233 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -436,15 +436,15 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
>  	/*
>  	 * Wait for transfer to complete before returning. Otherwise, the EmitStop
>  	 * request might be sent when the transfer is not complete.
>  	 */
>  	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
>  						SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000);

[Severity: High]
This is a pre-existing issue, but could this polling loop cause a bus
stall and deadlock if an I3C device transmits an IBI payload larger than
the controller's FIFO size of 16 bytes? If the FIFO becomes full, the
hardware will stall the bus waiting for the CPU to drain it. However, the
driver blocks in this timeout loop waiting for completion before it begins
reading from the FIFO, meaning the transaction can never complete.

>  	if (ret) {
>  		dev_err(master->dev, "Timeout when polling for COMPLETE\n");
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does this early return path leave stale
bytes in the RX FIFO? The hardware RX FIFO is not explicitly flushed with a
flush command here, which means any partially received data might remain in
the controller and corrupt the next I3C transfer.

>  	}
>  
>  	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);

[Severity: High]
This isn't a bug introduced by this patch, but what happens if a device
that did not request an IBI triggers one while IBIs are globally enabled
by another device? In that case, both data->ibi_pool and dev->ibi would
be NULL. Does this code need to verify that the device actually requested
an IBI before attempting to allocate a slot from data->ibi_pool?

>  	if (!slot) {
>  		dev_dbg(master->dev, "No free ibi slot, drop the data\n");
>  		writel(SVC_I3C_MDATACTRL_FLUSHRB, master->regs + SVC_I3C_MDATACTRL);
>  		return -ENOSPC;
>  	}
>  
>  	slot->len = 0;
>  	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 since the patch now explicitly
dereferences dev->ibi here, does this introduce an immediate crash point
if a spurious IBI arrives from a device that hasn't requested one? If
dev->ibi is NULL, dereferencing max_payload_len will cause a panic.

>  		mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
>  		count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
> +		count = min(count, dev->ibi->max_payload_len - slot->len);
>  		readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
>  		slot->len += count;
>  		buf += count;
>  	}
>  
> +	/*
> +	 * The device may have sent more than the requested payload. Drop the
> +	 * extra bytes so they do not leak into the next transfer.
> +	 */
> +	if (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)))
> +		writel(SVC_I3C_MDATACTRL_FLUSHRB, master->regs + SVC_I3C_MDATACTRL);
> +
>  	master->ibi.tbq_slot = slot;
>  
>  	return 0;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed the driver appears to mix
stream accessors and register accessors for the same data FIFOs, which
can cause data corruption on big-endian architectures.

For example, in svc_i3c_master_read():

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_read() {
    ...
    in[offset + i] = readl(master->regs + SVC_I3C_MRDATAB);
    ...
}

And similarly in svc_i3c_master_write():

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_write() {
    ...
    writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
    ...
}

Because readl() and writel() perform an implicit CPU-to-device byteswap,
using them for 8-bit remainder or single-byte transfers shifts the valid
data to the MSB on big-endian architectures. Could this be updated to use
the appropriate stream accessors like readsb or writesb consistently?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178227747353.2931373.15868718612134648277@maoyixie.com?part=1

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