linux-kernel - RE: [PATCH v3] i3c: master: fix for SETDASA and DAA process

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <DM5PR07MB3196085F8628478E7E2F5FC8C15A0@DM5PR07MB3196.namprd07.prod.outlook.com>
Date:   Thu, 20 Aug 2020 18:16:14 +0000
From:   Parshuram Raju Thombare <pthombar@...ence.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
CC:     "bbrezillon@...nel.org" <bbrezillon@...nel.org>,
        "vitor.soares@...opsys.com" <vitor.soares@...opsys.com>,
        Przemyslaw Gaj <pgaj@...ence.com>,
        "linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Milind Parab <mparab@...ence.com>,
        "praneeth@...com" <praneeth@...com>
Subject: RE: [PATCH v3] i3c: master: fix for SETDASA and DAA process

>Hm, not sure that qualifies as a fix. The current implementation was
>correct, it was just reserving a slot in the device table for devices
>that didn't have an init address or on which SETDASA failed.
If I3C controllers like ours use hardware slots to store slave devices info, 
due to limited available slots this can cause issue. 
 If some slots are lost due to
1. only init_dyn_addr and no static_addr in DT 
OR
2. SETDASA failed
at the end of DAA some devices may be left without dyn_addr allocated from master
and hence can't work properly.
I think during our discussion we recognized this change as a bug.
That is the reason I added fixes tag, but if you think otherwise I can remove this tag.
 
>> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
>> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller
>That function now does more than just assigning a dynamic address: it
>also creates the i3c_dev_desc. It should be renamed accordingly
>(i3c_master_early_i3c_dev_add() maybe).
Ok

>You should reserve the address before calling
>i3c_master_pre_assign_dyn_addr():
>
>		/*
>		 * We don't attach devices which are not addressable
>		 * (no static_addr and dyn_addr) and devices with
>		 * static_addr but no init_dyn_addr will participate in DAA.
>		 */
>		if (!i3cboardinfo->init_dyn_addr ||
>		    !i3cboardinfo->static_addr)
>			continue;
Don't we want to cover the case when only init_dyn_addr is present ?
I am not sure if we can't have init_dyn_addr without static_addr.
May be what we need is 
		if (!i3cboardinfo->init_dyn_addr)
			continue;

		ret = i3c_bus_get_addr_slot_status(&master->bus,
						   i3cboardinfo->init_dyn_addr);
		if (ret != I3C_ADDR_SLOT_FREE) {
			ret = -EBUSY;
			goto err_rstdaa;
		}

		i3c_bus_set_addr_slot_status(&master->bus,
					     i3cboardinfo->init_dyn_addr,
					     I3C_ADDR_SLOT_I3C_DEV);

		if (i3cboardinfo->static_addr)
			i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
IMHO this is functionally same to what I sent. Just that init_dyn_addr is reserved before,
and we leverage the change in reattach to bypass failure due to second attempt
to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr().