Re: [PATCH v3 06/13] i3c: master: Add support for devices without PID

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] Pre-existing use-after-free and double-free on device registration failure.
- [High] Validation bypass allows SETDASA devices with a missing or zero PID to be incorrectly registered.
- [Medium] PID validation is completely bypassed for SETAASA devices, allowing malformed PIDs to be accepted and exposed.
- [Medium] Dead code and logic flaws in device matching and duplicate detection for dynamically discovered devices.
--

commit dbd7e3bde8caae211d2b1844852f3a001da04d22
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: Add support for devices without PID

This commit allows devices without a 48-bit PID to register using their
static address. It primarily supports SETAASA address assignment as per the
I3C specification where a PID is not strictly required.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index de72317c9a5c73..fbe8710c4c56f8 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1842,8 +1842,17 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		desc->dev->dev.type = &i3c_device_type;
>  		desc->dev->dev.bus = &i3c_bus_type;
>  		desc->dev->dev.release = i3c_device_release;
> -		dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> -			     desc->info.pid);
> +
> +		/*
> +		 * For devices without PID (e.g., SETAASA devices), use
> +		 * static address for naming instead.
> +		 */
> +		if (desc->info.pid)
> +			dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> +				     desc->info.pid);
> +		else
> +			dev_set_name(&desc->dev->dev, "%d-%02x", master->bus.id,
> +				     desc->info.static_addr);
>  
>  		if (desc->boardinfo)
>  			device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);

This problem wasn't introduced by this patch, but does the error path further
down in this function leave a dangling pointer?

When device_register() fails, put_device() is called, which triggers
i3c_device_release() and frees desc->dev. However, desc->dev is not set to
NULL.

Later, during master teardown or driver unbind,
i3c_master_unregister_i3c_devs() iterates over the bus devices and checks
if (!i3cdev->dev) which will incorrectly pass. Could this lead to a
use-after-free and a subsequent double-free?

[ ... ]

> @@ -2288,8 +2297,18 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
>  	struct i3c_dev_boardinfo *i3cboardinfo;
>  
>  	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> -		if (i3cdev->info.pid != i3cboardinfo->pid)
> -			continue;
> +		/*
> +		 * For devices without PID (e.g., SETAASA devices), match by
> +		 * static address. For devices with PID, match by PID.
> +		 */
> +		if (i3cboardinfo->pid) {
> +			if (i3cdev->info.pid != i3cboardinfo->pid)
> +				continue;
> +		} else {
> +			if (!i3cboardinfo->static_addr ||
> +			    i3cdev->info.static_addr != i3cboardinfo->static_addr)
> +				continue;
> +		}

Is it possible for the static address match to succeed here?

This function appears to be called by i3c_master_add_i3c_dev_locked() for
devices dynamically discovered via the ENTDAA procedure. During ENTDAA, the
static address is not retrieved, meaning i3cdev->info.static_addr would be
zero.

Could this cause the match against non-zero SETAASA static addresses
to always fail?

[ ... ]

> @@ -2303,8 +2322,12 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	struct i3c_master_controller *master = i3c_dev_get_master(refdev);
>  	struct i3c_dev_desc *i3cdev;
>  
> +	if (!refdev->info.pid)
> +		return NULL;
> +

Can refdev->info.pid ever be zero here?

Similar to attach_boardinfo, this duplicate search seems to be used during
ENTDAA discovery. During ENTDAA, the hardware PID is always retrieved, so
refdev->info.pid is never zero. Devices statically configured with SETAASA
bypass these functions entirely.

[ ... ]

> @@ -2604,9 +2627,15 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>  
>  	boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>  
> -	if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> -	    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> -		return -EINVAL;
> +	/* For SETAASA devices, validate the static address instead of PID */
> +	if (boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		if (!boardinfo->static_addr)
> +			return -EINVAL;
> +	} else {
> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> +			return -EINVAL;
> +	}

Does this bypass PID validation entirely for SETAASA devices, even if a
PID is provided?

If a device tree node explicitly provides a PID in its reg property for a
SETAASA device, it skips the reserved upper bits check. The unvalidated PID
is then stored in boardinfo->pid, and later used to name the device via
dev_set_name(). Could this expose malformed data to userspace in sysfs?

Also, if the mipi-i3c-static-method is SETDASA (or any method other than
SETAASA) and the device has a missing or zero PID, the execution reaches
this else block. Because boardinfo->pid is zero, the bitwise validation
mathematically evaluates to zero and passes.

Does this allow SETDASA devices to bypass validation and be registered
without a valid PID, violating the I3C specification?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423085718.70762-1-akhilrajeev@nvidia.com?part=6