Re: [PATCH] i3c: master: svc: report timeout waiting for STOP idle

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing synchronization between IBI processing and device detachment causes Use-After-Free.
- [Critical] Stack Buffer Overflow during Dynamic Address Assignment (DAA).
- [Critical] Heap Buffer Overflow in IBI payload reading loop.
- [High] Use of sleeping polling macro in atomic/hardirq context.
- [High] DDR write payload is written twice to the FIFO when SVC_I3C_QUIRK_FIFO_EMPTY is active.
- [High] Hard lockup (infinite loop) in hardirq handler if hardware reports RX pending without bytes.
- [High] Endianness Portability Bug (Mixed I/O accessors on FIFO).
--

commit f5ce1c99dd938b33fc88fc935174301f58c2a87a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

i3c: master: svc: report timeout waiting for STOP idle

The commit modifies svc_i3c_master_xfer() to capture the return value
of readl_poll_timeout() when waiting for the controller state to become
idle. It routes the error through the cleanup path to ensure the caller
observes the failed transfer.

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 93805df8a9406..ae38ceee5abf8 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1488,8 +1488,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  			svc_i3c_master_emit_force_exit(master);
>  
>  		/* Wait idle if stop is sent. */
> -		readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> -				   SVC_I3C_MSTATUS_STATE_IDLE(reg), 0, 1000);
> +		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +					 SVC_I3C_MSTATUS_STATE_IDLE(reg),
> +					 0, 1000);

[Severity: High]
This isn't a bug introduced by this patch, but does this code use a
sleeping polling macro in an atomic context?

The readl_poll_timeout macro uses ktime_get() for non-zero timeouts.
Calling this from an atomic context, such as while holding a spinlock
with interrupts disabled, can cause kernel deadlocks when
CONFIG_DEBUG_ATOMIC_SLEEP is enabled.

This also appears in svc_i3c_master_handle_ibi() where
readl_relaxed_poll_timeout() is used inside a hardirq handler.

> +		if (ret)
> +			goto cleanup;
>  	}
>  
>  	return 0;
> @@ -1500,6 +1503,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	else
>  		svc_i3c_master_emit_force_exit(master);
>  
> +cleanup:
>  	svc_i3c_master_clear_merrwarn(master);
>  	svc_i3c_master_flush_fifo(master);

While reviewing this file, I noticed a few other pre-existing issues:

[Severity: High]
This is a pre-existing issue, but does this code write the DDR payload to
the FIFO twice when the SVC_I3C_QUIRK_FIFO_EMPTY quirk is active?

In svc_i3c_master_xfer(), if the transfer is SVC_I3C_MCTRL_TYPE_DDR and
is a write, the full payload is pushed into the FIFO:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
    if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
        writel(rnw_cmd, master->regs + SVC_I3C_MWDATAB);
        if (!rnw) {
            ret = svc_i3c_master_write(master, out, xfer_len);
    ...
}

However, xfer_len is not modified. Later, the quirk workaround triggers
and pushes the same payload bytes into the FIFO again:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
        if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
            ...
            writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code mix stream and register
I/O accessors on the same FIFO, potentially causing data corruption on
big-endian architectures?

The subsystem architecture guidelines explicitly forbid mixing accessors
like writesb() and writel() on the same FIFO address because register
accessors perform implicit byte-swapping while stream accessors do not.

In svc_i3c_master_xfer(), both are mixed for bulk and remainder writes:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
            writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
            /* Mark END bit if this is the last byte */
            writel(out[len - 1] | end, master->regs + SVC_I3C_MWDATAB);
    ...
}

Similarly, readsb() is used in svc_i3c_master_handle_ibi() on the RX FIFO,
whereas svc_i3c_master_read() uses readl() on the same FIFO.

[Severity: Critical]
This is a pre-existing issue, but is there missing synchronization between
IBI processing and device detachment that could cause a use-after-free?

In svc_i3c_master_ibi_isr(), the device descriptor is fetched under
master->ibi.lock, but the lock is dropped before handling the IBI:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_ibi_isr() {
    ...
    spin_lock(&master->ibi.lock);
    dev = master->ibi.slots[ibiaddr];
    spin_unlock(&master->ibi.lock);
    ...
    svc_i3c_master_handle_ibi(master, dev);
    ...
}

If the I3C core detaches the device concurrently,
svc_i3c_master_detach_i3c_dev() can free the device data without
synchronizing against the IRQ handler:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_detach_i3c_dev() {
    ...
    kfree(data);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this code lack bounds checking on
dev_nb, potentially causing a stack buffer overflow?

The svc_i3c_master_do_daa_locked() function defines fixed-size arrays on
the stack for up to SVC_I3C_MAX_DEVS (32) devices:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_do_daa_locked() {
    u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
    unsigned int dev_nb = 0, last_addr = 0, dyn_addr = 0;
    ...
}

The loop assigns addresses to responding devices and writes to these arrays
using dev_nb without verifying it hasn't exceeded the array bounds:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_do_daa_locked() {
    ...
    addrs[dev_nb] = dyn_addr;
    ...
    dev_nb++;
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does the loop draining the hardware
RX FIFO use a hardcoded bound that could exceed the dynamically allocated
buffer size?

The slot->data buffer is allocated based on the device's max_payload_len,
which can be small. However, the loop condition checks against
SVC_I3C_FIFO_SIZE (16 bytes):

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_handle_ibi() {
    ...
    while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
           slot->len < SVC_I3C_FIFO_SIZE) {
        ...
        count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
        readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
        slot->len += count;
        ...
}

Because the check evaluates slot->len < 16 before adding count (which
can also be up to 16 bytes), up to 31 bytes could be written to a buffer
that might only be 1 or 2 bytes long.

[Severity: High]
This is a pre-existing issue, but could this loop cause a hard lockup in
the hardirq handler if the hardware reports RXPEND is true but RXCOUNT
is 0?

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_handle_ibi() {
    ...
    while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
           slot->len < SVC_I3C_FIFO_SIZE) {
        ...
        count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
        readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
        slot->len += count;
        buf += count;
    }
    ...
}

The loop relies entirely on the hardware state to terminate. If RXCOUNT
is 0, count will be 0, slot->len will not increase, and the loop will
spin indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623060821.23238-1-pengpeng@iscas.ac.cn?part=1