Re: [PATCH RFC 2/7] i3c: master: Report wakeup events for IBIs

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

New issues:
- [High] Unconditional dereference of `dev->dev` in `i3c_master_queue_ibi()` causes a NULL pointer dereference or use-after-free when an IBI interrupt triggers during master controller teardown.

Pre-existing issues:
- [High] Dangling pointer left in `desc->dev` when `device_register()` fails, leading to UAF and double free during teardown.
- [High] A TOCTOU race in `i3c_master_queue_ibi()` allows `dev->ibi` to be accessed and modified after it has been freed by a concurrent driver unbind.
--

commit 88b3910e92d2416ef1cb5400ef8ee3bbdd8200c5
Author: Adrian Hunter <[email protected]>

i3c: master: Report wakeup events for IBIs

This commit allows an I3C device configured as a wakeup source to wake the
system via an In-Band Interrupt (IBI). When an IBI is queued, a wakeup event is
recorded with a 100 ms processing interval to allow the driver time to process
it.

> 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
> @@ -2994,6 +2994,9 @@ static void i3c_master_unregister_i3c_devs(struct i3c_master_controller *master)
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at how devices are
unregistered here in i3c_master_unregister_i3c_devs():

    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;

Is there a risk of a double free if device_register() fails earlier in
i3c_master_register_new_i3c_devs()?

If the error path in registration calls put_device(&desc->dev->dev) but fails
to set desc->dev to NULL, this teardown loop will later find the dangling
pointer, perform a use-after-free write setting desc to NULL, and call
put_device() again on freed memory.

>  
> +/* Approximate time for IBI handler to run */
> +#define I3C_WAKEUP_PROCESSING_TIME_MS 100
> +
>  /**
>   * i3c_master_queue_ibi() - Queue an IBI
>   * @dev: the device this IBI is coming from
> @@ -3007,6 +3010,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot)
>  	if (!dev->ibi || !slot)
>  		return;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for dev->ibi to
be freed concurrently?

If i3c_dev_free_ibi_locked() executes kfree(dev->ibi) and sets dev->ibi = NULL
during device unbind without calling synchronize_irq() to wait for running
interrupts to finish:

Could an ISR running concurrently on another CPU pass the if (!dev->ibi) check
above, get delayed, and then access dev->ibi in the atomic_inc() below after
it has been freed?

>  
> +	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 or use-after-free risk
during master controller teardown?

During unregistration, i3c_master_unregister_i3c_devs() nullifies
i3cdev->dev->desc and sets i3cdev->dev = NULL. Because the unregistration code
nullifies dev->desc early, the driver's .remove() callback might silently fail
to clean up and disable the IBI hardware interrupt.

If the interrupt remains active and fires, the ISR will call
i3c_master_queue_ibi(). Since this new code unconditionally dereferences
dev->dev without verifying it is not NULL, wouldn't this cause an immediate
kernel oops?

>  	atomic_inc(&dev->ibi->pending_ibis);
>  	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