Re: [PATCH 3/3] i3c: master: amd: Add hot-join support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver duplicates the I3C core framework's Hot-Join work mechanism but conditionally cancels it, leading to a Use-After-Free during driver unbind.
--

--- Patch [3]: [PATCH 3/3] i3c: master: amd: Add hot-join support ---
commit 06fad25ac0b3ab8838ac7e9c3c0f9c33f86244dc
Author: Shubham Patil <[email protected]>

i3c: master: amd: Add hot-join support

This patch adds hot-join support for the AMD AXI I3C master controller.
By default, the hot-join acknowledgment is disabled, but it can be enabled
via sysfs. A hot-join event is serviced by re-running DAA from a work
item so the i3c core enumerates the new device.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> --- a/drivers/i3c/master/amd-i3c-master.c
> +++ b/drivers/i3c/master/amd-i3c-master.c
[ ... ]
> @@ -221,12 +228,14 @@ struct xi3c_master {
>  	bool xfer_resp_valid;
>  	int irq;
>  	bool ibi_capable;
> +	bool hj_capable;
>  	struct i3c_master_controller_ops ops;
>  	struct {
>  		spinlock_t lock; /* protects slots[] against the IBI handler */
>  		struct i3c_dev_desc *slots[XI3C_MAX_DEVS];
>  		unsigned int enabled_count;
>  	} ibi;
> +	struct work_struct hj_work;
>  };

[Severity: High]
Could this custom hj_work bypass the safety checks in the core framework?

The I3C core provides i3c_master_queue_hotjoin() which safely handles
queuing, checks the shutting_down flag, and guarantees unconditional
cancellation during i3c_master_unregister(). Defining a custom work item
here bypasses those safeguards.

[ ... ]
> @@ -1474,6 +1483,40 @@ static void xi3c_master_handle_ibi(struct xi3c_master *master)
[ ... ]
> +static void xi3c_master_hj_work(struct work_struct *work)
> +{
> +	struct xi3c_master *master = container_of(work, struct xi3c_master,
> +						  hj_work);
> +
> +	i3c_master_do_daa(&master->base);
> +}
> +
>  static irqreturn_t xi3c_master_irq_handler(int irq, void *dev_id)
>  {
>  	struct xi3c_master *master = dev_id;
> @@ -1494,6 +1537,9 @@ static irqreturn_t xi3c_master_irq_handler(int irq, void *dev_id)
>  			xi3c_master_handle_ibi(master);
>  	}
>  
> +	if (status & XI3C_INTR_HJ_MASK)
> +		queue_work(master->base.wq, &master->hj_work);
> +
>  	return IRQ_HANDLED;
>  }

[Severity: High]
Can this unconditionally queue the work even if hj_capable is false?

If the device tree lacks Hot-Join support (hj_capable is false), but the
hardware status register still records a Hot-Join event (for instance, from
an I3C target issuing a request while an IBI interrupt triggers the handler),
this will queue hj_work since master->hj_capable is not checked here.

[ ... ]
> @@ -1588,6 +1647,12 @@ static void xi3c_master_remove(struct platform_device *pdev)
>  	if (master->ibi_capable)
>  		disable_irq(master->irq);
>  
> +	if (master->hj_capable) {
> +		/* Stop new Hot-Join IRQs before flushing, so the work can't be re-queued. */
> +		xi3c_master_disable_hotjoin(&master->base);
> +		cancel_work_sync(&master->hj_work);
> +	}
> +
>  	i3c_master_unregister(&master->base);
>  }

[Severity: High]
Could this lead to a use-after-free during driver unbind?

If the work was queued while hj_capable is false, this conditional block is
skipped and the work is never cancelled. The devres framework will free the
master structure, but the work remains on the workqueue and will access freed
memory when it executes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c