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

[sashiko-bot@kernel.org]

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