Re: [PATCH v3 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:
- [High] Eagerly draining the shared RX FIFO during a polled read transfer consumes in-flight IBI payloads, corrupting both the IBI and the transfer data.
- [High] Freeing the IBI pool before the workqueue is flushed causes a UAF and NULL pointer dereference if a stray IBI is queued after disable.
--

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

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

Add support for receiving and dequeueing I3C in-band interrupts. IBI
support is optional at synthesis time, so the ops are assembled at probe
time. The controller interrupt carries IBI events only.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> index ef5ad5abb788c..8a8ab3cd2fa40 100644
> --- a/drivers/i3c/master/amd-i3c-master.c
> +++ b/drivers/i3c/master/amd-i3c-master.c

[ ... ]

> @@ -606,14 +678,21 @@ static int xi3c_master_read(struct xi3c_master *master, struct xi3c_cmd *cmd)
>  		return ret;
>  	}
>  
> -	if (!(status_reg & XI3C_RD_FIFO_NOT_EMPTY_MASK))
> +	/*
> +	 * No data queued means this read produced none, unless what is queued
> +	 * is an IBI response that raced with it; xi3c_xfer_resp_available()
> +	 * consumes that case so the wait for our own data can continue.
> +	 */
> +	if (!(status_reg & XI3C_RD_FIFO_NOT_EMPTY_MASK) &&
> +	    xi3c_xfer_resp_available(master))
>  		return 0;
>  
>  	timeout = jiffies + msecs_to_jiffies(XI3C_XFER_TIMEOUT_MS);
>  
>  	/* 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)) {

[Severity: High]
Could this loop eagerly consume in-flight IBI payloads and corrupt both the
IBI and the transfer data?

If a hardware target asserts an IBI right at the start of a read transfer,
its payload might stream into the shared RD_FIFO. Since the IBI's response
word is not posted to RESP_STATUS_FIFO until the transaction finishes, it
appears xi3c_xfer_resp_available() could return false here.

The polling loop would then execute xi3c_master_rd_from_rx_fifo(), blindly
draining the untagged IBI bytes from RD_FIFO into the transfer's buffer
before validating ownership. Later, when the IBI processing path
(xi3c_master_process_ibi) runs, it expects its payload to still be in the
RX FIFO and may underflow the FIFO or steal the transfer's actual data.

>  		if (time_after(jiffies, timeout)) {
>  			dev_err(master->dev, "XI3C read timeout\n");
>  			return -EIO;
>  		}
>  		xi3c_master_rd_from_rx_fifo(master, cmd);
>  		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_free_ibi(struct i3c_dev_desc *dev)
> +{
> +	struct xi3c_i3c_dev_data *data = i3c_dev_get_master_data(dev);
> +	struct i3c_master_controller *m = i3c_dev_get_master(dev);
> +	struct xi3c_master *master = to_xi3c_master(m);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&master->ibi.lock, flags);
> +	master->ibi.slots[data->ibi_slot] = NULL;
> +	spin_unlock_irqrestore(&master->ibi.lock, flags);
> +
> +	/*
> +	 * Clearing the slot above keeps the IBI handler from finding this
> +	 * device, but a handler already running may be copying a payload into
> +	 * the pool. Wait for it to finish before the pool is freed below.
> +	 */
> +	synchronize_irq(master->irq);
> +
> +	i3c_generic_ibi_free_pool(data->ibi_pool);
> +	i3c_dev_set_master_data(dev, NULL);
> +	kfree(data);

[Severity: High]
Does freeing the IBI pool here before the workqueue is flushed create a
use-after-free and NULL pointer dereference risk?

During device teardown, it looks like an asynchronous IBI could race
with device removal. If the core calls xi3c_master_disable_ibi() and then
proceeds to call xi3c_master_free_ibi(), an IBI interrupt might concurrently
fire.

The IRQ handler calling xi3c_master_process_ibi() could unconditionally queue
a work item to dev->ibi->wq. By the time the core calls destroy_workqueue(),
which flushes the queue, the work item would reside in freed memory.

Furthermore, when the flushed work executes, it calls
xi3c_master_recycle_ibi_slot() which would dereference the now-NULL master
data:

xi3c_master_recycle_ibi_slot() {
    struct xi3c_i3c_dev_data *data = i3c_dev_get_master_data(dev);
    i3c_generic_ibi_recycle_slot(data->ibi_pool, slot);
}

> +}

[ ... ]

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