Re: [PATCH v4 2/3] i3c: master: amd: Add support for in-band interrupts

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Polling the shared RX FIFO during a read transfer steals an in-flight IBI's payload data, causing data corruption.

Pre-existing issues:
- [High] Unaligned pointer dereference in FIFO I/O accessors causes undefined behavior.
--

commit b1cc4b97eb8d4b5fc534199add697a3f1a306c98
Author: Shubham Patil <shubhamsanjay.patil@amd.com>

i3c: master: amd: Add support for in-band interrupts

This commit adds support for receiving and dequeueing I3C in-band interrupts.
The ops are assembled at probe and callbacks registered when the appropriate
property is present. IBIs share the response and read FIFOs with normal
transfers and are tagged with TID 0x0F.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> index ef5ad5abb788c..a50445530a430 100644
> --- a/drivers/i3c/master/amd-i3c-master.c
> +++ b/drivers/i3c/master/amd-i3c-master.c
[ ... ]
> @@ -437,13 +606,19 @@ static int xi3c_master_read(struct xi3c_master *master, struct xi3c_cmd *cmd)
[ ... ]
>  	/* Read data from rx fifo */
> -	while (cmd->rx_len > 0 && !xi3c_is_resp_available(master)) {
> +	while (cmd->rx_len > 0 && !xi3c_xfer_resp_available(master)) {
>  		if (time_after(jiffies, timeout)) {
>  			dev_err(master->dev, "XI3C read timeout\n");
>  			return -EIO;
>  		}
>  		xi3c_master_rd_from_rx_fifo(master, cmd);

[Severity: Critical]
Does this code steal payload bytes from an in-flight IBI when polling the
shared RX FIFO during a read transfer?

If an IBI is ACKed just before a read transfer starts, its payload begins
filling the RX FIFO while the IBI is still on the bus. Because the IBI hasn't
completed, its response word is not yet in the RESP FIFO.

The read transfer's polling loop in xi3c_master_read() sees RD_FIFO_NOT_EMPTY
and eagerly drains the RX FIFO, taking the IBI payload into cmd->rx_buf.
When the IBI completes and its response finally appears,
xi3c_master_process_ibi() will expect the payload to still be in the RX FIFO,
but instead consumes the read transfer's data, permanently desynchronizing
the RX FIFO.

>  		usleep_range(XI3C_POLL_INTERVAL_US, 2 * XI3C_POLL_INTERVAL_US);
>  	}
[ ... ]
> @@ -1052,9 +1243,338 @@ static void xi3c_master_bus_cleanup(struct i3c_master_controller *m)
[ ... ]
> +static void xi3c_master_process_ibi(struct xi3c_master *master, u32 resp, u8 da)
> +{
[ ... ]
> +	slot->len = 0;
> +	if (len) {
> +		xi3c_readl_fifo(master->membase + XI3C_RD_FIFO_OFFSET,
> +				slot->data, len);

[Severity: High]
This is a pre-existing issue, but does passing slot->data to
xi3c_readl_fifo() cause an unaligned pointer dereference?

i3c_generic_ibi_alloc_pool() allocates a single contiguous buffer for all IBI
slots. If max_payload_len is not a multiple of 4, the slot->data pointers for
subsequent slots will not be 4-byte aligned.

When this unaligned slot->data pointer is passed to xi3c_readl_fifo(), it
eventually calls xi3c_readsl_be(). That accessor casts the void * buffer to
a u32 * and dereferences it directly without using put_unaligned():

drivers/i3c/master/amd-i3c-master.c:xi3c_readsl_be() {
    u32 *buf = buffer;
    while (count--)
        *buf++ = ioread32be(addr);
}

This can cause a kernel crash due to an alignment fault on architectures that
strictly enforce alignment.

> +		slot->len = len;
> +	}
> +
> +	i3c_master_queue_ibi(dev, slot);
> +	spin_unlock_irqrestore(&master->ibi.lock, flags);
> +
> +	return;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909071340.3649271-1-shubhamsanjay.patil@amd.com?part=2