Re: [PATCH] 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 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