Re: [PATCH v6 05/12] i3c: master: Add support for devices without PID

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The static address matching logic for devices without a PID in `i3c_master_attach_boardinfo` is unreachable and contains a structurally impossible condition.
- [Medium] Devices requiring a PID can now be registered with an invalid Manufacturer ID of 0.
--

--- Patch [5]: [PATCH v6 05/12] i3c: master: Add support for devices without PID ---
commit 671c3fae66002e874a13051a9c956e9987d76674
Author: Akhil R <[email protected]>

i3c: master: Add support for devices without PID
    
Devices using SETAASA for address assignment are not required to have
a 48-bit PID according to the I3C specification. Allow such devices to
register and use the static address where PID was required.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index b18dda89c4737..7b2f819bf4437 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -2389,8 +2398,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;
> +		}

[Severity: Medium]
Will this static address matching logic ever evaluate to false?

Since i3c_master_attach_boardinfo() is only called during the dynamic
address assignment (ENTDAA) phase, i3cdev->info.static_addr is inherently 0
in this context.

Because i3cboardinfo->static_addr is validated to be non-zero at creation,
the condition i3cdev->info.static_addr != i3cboardinfo->static_addr will
evaluate to 0 != non-zero, which is always true.

This means the loop will always continue, making this branch effectively
dead code that fails to perform the intended matching.

>  
>  		i3cdev->boardinfo = i3cboardinfo;
>  		i3cdev->info.static_addr = i3cboardinfo->static_addr;

[ ... ]

> @@ -2848,9 +2871,16 @@ 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 ||
> +		    (boardinfo->pid & GENMASK_ULL(63, 48)) ||
> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> +			return -EINVAL;
> +	}
>  
>  	boardinfo->init_dyn_addr = init_dyn_addr;
>  	boardinfo->fwnode = fwnode_handle_get(fwnode);
> @@ -2873,10 +2903,10 @@ static int i3c_master_add_of_dev(struct i3c_master_controller *master,
>  		return ret;
>  
>  	/*
> -	 * The manufacturer ID can't be 0. If reg[1] == 0 that means we're
> -	 * dealing with an I2C device.
> +	 * I3C device should have either the manufacturer ID specified or the
> +	 * address discovery method specified. Else treat it as an I2C device.
>  	 */
> -	if (!reg[1])
> +	if (!reg[1] && !fwnode_property_present(fwnode, "mipi-i3c-static-method"))
>  		ret = i3c_master_add_i2c_boardinfo(master, fwnode, reg);
>  	else
>  		ret = i3c_master_add_i3c_boardinfo(master, fwnode, reg);

[Severity: Medium]
Could this change allow devices requiring a PID to be registered with an
invalid Manufacturer ID of 0?

By permitting reg[1] == 0 when mipi-i3c-static-method is present in
i3c_master_add_of_dev(), it appears we can bypass the strict Manufacturer ID
validation in i3c_master_add_i3c_boardinfo().

If the method is SETDASA, which still requires a PID, boardinfo->pid is set
to ((u64)0 << 32) | reg[2].  If reg[2] is non-zero, !boardinfo->pid is false.

Would this result in successfully registering the device despite the zero
Manufacturer ID, violating the MIPI I3C specification?

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

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