Re: [PATCH V5 09/14] i3c: master: Report wakeup events for IBIs

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

New issues:
- [High] Unprotected access to `dev->dev` in `i3c_master_queue_ibi()` introduces a NULL pointer dereference or Use-After-Free during concurrent device teardown.

Pre-existing issues:
- [High] Preexisting lockless access to `dev->ibi` in `i3c_master_queue_ibi()` leads to a Use-After-Free (UAF) if an IBI is handled concurrently with IBI freeing.
--

--- Patch [9]: [PATCH V5 09/14] i3c: master: Report wakeup events for IBIs ---
commit b0c07417134eb0c8209460a9c252faf745d3feee
Author: Adrian Hunter <[email protected]>
Subject: i3c: master: Report wakeup events for IBIs

This commit modifies the I3C master subsystem to report wakeup events for
devices configured as wakeup sources. It queues a wakeup event when an
In-Band Interrupt (IBI) is processed by i3c_master_queue_ibi().

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 2616354dbe545..6c53414919445 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -3397,6 +3400,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot)
>  	if (!dev->ibi || !slot)
>  		return;
>  
> +	if (device_may_wakeup(&dev->dev->dev))
> +		pm_wakeup_event(&dev->dev->dev, I3C_WAKEUP_PROCESSING_TIME_MS);
> +
[Severity: High]
Does this code introduce a potential NULL pointer dereference or
use-after-free if a concurrent teardown unregisters the device?

Since i3c_master_queue_ibi() runs locklessly from the master's IRQ handler,
could a concurrent teardown thread reach i3c_master_unregister_i3c_devs():

	put_device(&i3cdev->dev->dev);
	i3cdev->dev = NULL;

while the IRQ handler is between the !dev->ibi check and this new wakeup
check?

If the controller's disable_ibi callback does not synchronize with the IRQ
handler using synchronize_irq(), the teardown thread could see
pending_ibis == 0 and proceed to unbind the device, which would cause
dereferencing dev->dev->dev here to panic.

>  	atomic_inc(&dev->ibi->pending_ibis);
>  	queue_work(dev->ibi->wq, &slot->work);
[Severity: High]
This is a pre-existing issue, but can this lockless access to dev->ibi cause
a use-after-free during concurrent device unbinding?

If the IRQ handler has passed the !dev->ibi check but hasn't yet reached
this atomic_inc(), a concurrent thread executing i3c_dev_free_ibi_locked()
could disable IBIs.

Because pending_ibis has not been incremented yet, it would see
pending_ibis == 0 and proceed to free the IBI resources without waiting:

	kfree(dev->ibi);
	dev->ibi = NULL;

When the IRQ handler resumes, it would execute atomic_inc() and queue_work(),
writing to the freed dev->ibi structure.

>  }

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

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