Re: [PATCH RFC RESEND 4/7] i3c: master: Reject IBI requests from non-IBI-capable devices

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

Pre-existing issues:
- [Critical] Lockless access to `dev->ibi` from interrupt context races with non-atomic pointer clearing, causing a potential Use-After-Free.
- [High] Recursive read lock acquisition of `bus->lock` leading to potential AB-BA deadlock with writers.
- [High] Memory and resource leak of the IBI workqueue on the error path of `i3c_dev_request_ibi_locked`.
--

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

i3c: master: Reject IBI requests from non-IBI-capable devices

i3c_device_request_ibi() does not verify that a device advertises IBI
support before attempting to set up IBI handling. Add a check for
I3C_BCR_IBI_REQ_CAP and fail with -EOPNOTSUPP when IBI support is not
reported by the device.

> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index 101eaa77de686..307ca6dc04ca0 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -205,12 +205,14 @@ int i3c_device_request_ibi(struct i3c_device *dev,
>  		return ret;
>  
>  	i3c_bus_normaluse_lock(dev->bus);

[Severity: High]
This is a pre-existing issue, but can this cause a deadlock with writers
if a writer arrives between two down_read() calls?

In drivers/i3c/master.c:i3c_master_reg_work_fn(), the first read lock is
acquired before triggering synchronous device registration:

	i3c_bus_normaluse_lock(&master->bus);
	if (!master->shutting_down)
		i3c_master_register_new_i3c_devs(master);

This synchronous probe path then calls i3c_device_request_ibi(), which
recursively attempts to acquire the same lock. If a writer such as
i3c_bus_maintenance_lock() (which takes down_write) arrives between the
two down_read() calls, the second down_read() blocks to prevent writer
starvation, while the writer blocks waiting for the first reader.

> -	if (dev->desc) {
> +	if (!dev->desc) {
> +		ret = -ENOENT;
> +	} else if (!(dev->desc->info.bcr & I3C_BCR_IBI_REQ_CAP)) {
> +		ret = -EOPNOTSUPP;
> +	} else {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		ret = i3c_dev_request_ibi_locked(dev->desc, req);

[Severity: High]
This is a pre-existing issue, but does this code leak the workqueue on
the error path?

Looking at drivers/i3c/master.c:i3c_dev_request_ibi_locked():

	ibi->wq = alloc_ordered_workqueue(dev_name(i3cdev_to_dev(dev->dev)), WQ_MEM_RECLAIM);
	...
	ret = master->ops->request_ibi(dev, req);
	if (ret) {
		kfree(ibi);
		dev->ibi = NULL;
	}

If master->ops->request_ibi() fails, the ibi struct is freed but there
is no call to destroy_workqueue(ibi->wq), permanently leaking the memory
and associated kernel thread resources.


[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free
if an interrupt handler accesses dev->ibi concurrently?

In drivers/i3c/master.c:i3c_master_queue_ibi(), the hardware interrupt
handler reads dev->ibi locklessly to queue incoming IBIs:

	if (!dev->ibi || !slot)
		return;
	...
	atomic_inc(&dev->ibi->pending_ibis);
	queue_work(dev->ibi->wq, &slot->work);

However, in the error path of i3c_dev_request_ibi_locked() shown above,
and in i3c_dev_free_ibi_locked(), the memory is freed before the pointer
is cleared:

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

Because there is no synchronization, a concurrent interrupt on another
CPU can read the pointer, get delayed, and then dereference it after it
has been freed.

>  		mutex_unlock(&dev->desc->ibi_lock);
> -	} else {
> -		ret = -ENOENT;
>  	}
>  	i3c_bus_normaluse_unlock(dev->bus);
>

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

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