Re: [PATCH RFC RESEND 2/7] 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] Unconditional NULL pointer dereference of `dev->dev` in `i3c_master_queue_ibi` during device teardown

Pre-existing issues:
- [High] Lockless TOCTOU race leading to Use-After-Free of `dev->ibi` during concurrent IBI teardown
--

commit ffdd9b066a2bdadecaab205d187dab1a5ecff5ae
Author: Adrian Hunter <[email protected]>

i3c: master: Report wakeup events for IBIs

An I3C device configured as a wakeup source can wake the system by
generating an In-Band Interrupt (IBI).

When an IBI is queued for processing, record a wakeup event for the
device if wakeup is enabled.  Use a 100 ms processing interval to give
the I3C device driver time to process the IBI.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index a47e2924bb701..a36f9b617cbf7 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -3007,6 +3010,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 NULL pointer dereference if an interrupt arrives
during device teardown?

During I3C master controller unregistration, i3c_master_unregister_i3c_devs()
clears the device description and sets i3cdev->dev to NULL:

	i3cdev->dev->desc = NULL;
	if (device_is_registered(&i3cdev->dev->dev))
		device_unregister(&i3cdev->dev->dev);
	else
		put_device(&i3cdev->dev->dev);
	i3cdev->dev = NULL;

Since clearing dev->desc prevents the driver's remove callback from disabling
the hardware IBI, the IBI may still fire. If i3c_master_queue_ibi() executes
at this point, the newly added device_may_wakeup(&dev->dev->dev) will
dereference the NULL dev->dev pointer.

>  	atomic_inc(&dev->ibi->pending_ibis);

[Severity: High]
This is a pre-existing issue, but does this lockless check open a 
use-after-free race window?

If a hardware IBI interrupt is processed concurrently on another CPU, it can
evaluate if (!dev->ibi) as false, get preempted, and then access
dev->ibi->pending_ibis and dev->ibi->wq after they have been freed by
i3c_dev_free_ibi_locked():

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

This is possible for hardware controllers like dw-i3c-master that do not
implicitly synchronize with executing hardware IRQ handlers during their
disable op.

>  	queue_work(dev->ibi->wq, &slot->work);
>  }

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

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