Re: [PATCH 1/3] i3c: master: Update dev_nack_retry_count under maintenance lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting Use-After-Free (UAF) due to devres freeing the I3C master controller while its embedded `struct device` is still referenced.
--

commit d8d2b455dc4bd3d672311fc14258b4a9d0b83e8b
Author: Adrian Hunter <adrian.hunter@intel.com>
Subject: i3c: master: Update dev_nack_retry_count under maintenance lock

This commit protects master->dev_nack_retry_count against concurrent sysfs
updates by updating it while holding the bus maintenance lock. It also
combines adjacent return statements and updates the read path to hold the bus
normaluse lock.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 372d911ecbadc..05ea9e3c4d46f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -744,7 +744,14 @@ static DEVICE_ATTR_RW(hotjoin);
>  static ssize_t dev_nack_retry_count_show(struct device *dev,
>  					 struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	ssize_t ret;
> +
> +	i3c_bus_normaluse_lock(i3cbus);
> +	ret = sysfs_emit(buf, "%u\n", dev_to_i3cmaster(dev)->dev_nack_retry_count);

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying sysfs
attributes here, is there a pre-existing use-after-free risk related to how
the i3c_master_controller is allocated and freed?

Many I3C master drivers (e.g., dw-i3c-master.c) allocate their driver context
(which embeds struct i3c_master_controller) using devm_kzalloc(). During
unbind/removal, device_unregister(&master->dev) is called, which drops the
initial reference. If a user-space process holds a sysfs file open (like this
dev_nack_retry_count attribute), or if an attacker blocks in a sysfs read or
write using userfaultfd, the kobject reference count remains elevated.

The driver's remove function then returns, and the devres framework
automatically frees the driver context memory. Later, when the final reference
is dropped, the driver core calls i3c_masterdev_release():

drivers/i3c/master.c:i3c_masterdev_release() {
        struct i3c_master_controller *master = dev_to_i3cmaster(dev);
        struct i3c_bus *bus = dev_to_i3cbus(dev);

        if (master->wq)
                destroy_workqueue(master->wq);
        ...
}

Will this result in a use-after-free since the release function accesses the
master struct after its memory has already been freed by devres? Should the
memory for structures embedding a struct device be freed within the release
function instead of being tied to the driver unbind lifecycle via devres?

> +	i3c_bus_normaluse_unlock(i3cbus);
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616113752.196140-1-adrian.hunter@intel.com?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c