linux-kernel - Re: [PATCH v3 11/11] i3c: master: svc: fix possible assignment of the same address to two devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240823182416.33744666@xps-13>
Date: Fri, 23 Aug 2024 18:24:16 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Frank Li <Frank.Li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>, Boris Brezillon
 <boris.brezillon@...labora.com>, Parshuram Thombare <pthombar@...ence.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Boris Brezillon
 <bbrezillon@...nel.org>, Arnd Bergmann <arnd@...db.de>, Conor Culhane
 <conor.culhane@...vaco.com>, linux-i3c@...ts.infradead.org,
 linux-kernel@...r.kernel.org, imx@...ts.linux.dev, stable@...nel.org
Subject: Re: [PATCH v3 11/11] i3c: master: svc: fix possible assignment of
 the same address to two devices

Hi Frank,

Frank.Li@....com wrote on Mon, 19 Aug 2024 12:02:05 -0400:

> svc_i3c_master_do_daa() {
>     ...
>     for (i = 0; i < dev_nb; i++) {
>         ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
>         if (ret)
>             goto rpm_out;
>     }
> }
> 
> If two devices (A and B) are detected in DAA and address 0xa is assigned to
> device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked()
> for device A (addr: 0xa) could prevent device B (addr: 0xb) from being
> registered on the bus. The I3C stack might still consider 0xb a free
> address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A,
> causing both devices A and B to use the same address 0xb, violating the I3C
> specification.
> 
> The return value for i3c_master_add_i3c_dev_locked() should not be checked
> because subsequent steps will scan the entire I3C bus, independent of
> whether i3c_master_add_i3c_dev_locked() returns success.
> 
> If device A registration fails, there is still a chance to register device
> B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while
> retrieving device information.
> 
> Cc: stable@...nel.org
> Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 2010495906eb3..003565fddc261 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1042,11 +1042,8 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
>  		goto rpm_out;
>  
>  	/* Register all devices who participated to the core */
> -	for (i = 0; i < dev_nb; i++) {
> -		ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
> -		if (ret)
> -			goto rpm_out;
> -	}
> +	for (i = 0; i < dev_nb; i++)
> +		i3c_master_add_i3c_dev_locked(m, addrs[i]);

Makes sense, but please explain why don't check the return value in a
comment (your commit log is good).

>  
>  	/* Configure IBI auto-rules */
>  	ret = svc_i3c_update_ibirules(master);
> 

With the comment added,

Reviewed-by: Miquel Raynal <miquel.raynal@...tlin.com>

Thanks,
Miquèl