Re: [PATCH RFC RESEND 3/7] i3c: master: Add helper to query bus wakeup requirements

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

New issues:
- [High] Concurrent execution between `i3c_master_any_wakeup_enabled()` and the device registration worker causes data races on `desc->dev` because both paths use a read-side lock.

Pre-existing issues:
- [High] The `i3c_master_any_wakeup_enabled()` helper can dereference a dangling `desc->dev` pointer if a previous I3C device registration failed.
--

commit 3ed97d097ea9f23fefcb772811737adab713eb66
Author: Adrian Hunter <[email protected]>

i3c: master: Add helper to query bus wakeup requirements

This commit adds i3c_master_any_wakeup_enabled(), which iterates over the
devices on an I3C bus and reports whether any of them are enabled for system
wakeup. Controller drivers can use this helper to determine whether wakeup
support must remain available while the system is suspended.

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

[ ... ]

> +bool i3c_master_any_wakeup_enabled(struct i3c_master_controller *master)
> +{
> +	struct i3c_dev_desc *desc;
> +	bool wakeup = false;
> +
> +	i3c_bus_normaluse_lock(&master->bus);

[Severity: High]
Does this code risk a data race with the device registration worker?

Looking at i3c_master_reg_work_fn(), it allocates and initializes desc->dev
while holding this same read-side lock (i3c_bus_normaluse_lock). If the PCI
controller runtime suspends and calls this helper while the registration work
is executing, they can run concurrently since both only hold a read-side
lock.

Can this lead to a use-before-initialization if device_may_wakeup() is
called while the memory is still being initialized?

> +	i3c_bus_for_each_i3cdev(&master->bus, desc) {
> +		if (desc->dev && device_may_wakeup(&desc->dev->dev)) {

[Severity: High]
This is a pre-existing issue, but could this check trigger a use-after-free
if a previous device registration failed?

In i3c_master_register_new_i3c_devs(), if device_register() fails, the error
path drops the reference with put_device(&desc->dev->dev). This frees the
underlying memory, but desc->dev is never set to NULL.

If that happens, won't this loop observe a dangling non-NULL desc->dev
pointer and unconditionally dereference it during suspend?

> +			wakeup = true;
> +			break;
> +		}
> +	}
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return wakeup;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_any_wakeup_enabled);

-- 
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