Re: Fwd: [PATCH] [RFC] cmd: i2c: fix default address len for DM_I2C
Hello all,
On Tue, Aug 16, 2022 at 1:47 PM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Tue, 16 Aug 2022 at 13:50, Tim Harvey tharvey@gateworks.com wrote:
On Mon, Aug 15, 2022 at 3:16 PM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Mon, 15 Aug 2022 at 11:48, Tim Harvey tharvey@gateworks.com wrote:
On Sat, Aug 13, 2022 at 7:59 AM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote:
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?).
cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
Simon,
Here's some annotated debug prints added where chip_offset is passed/set: Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit DRAM: 1 GiB i2c_chip_of_to_plat gsc@20 offset_len=1 i2c_chip_of_to_plat pmic@69 offset_len=1 i2c_get_chip i2c@30a20000 0x51 offset_len=1 i2c_bind_driver i2c@30a20000 offset_len=1 i2c_set_chip_offset_len generic_51 offset_len=1 dm_i2c_read generic_51 offset=0 offset_len=1 i2c_setup_offset 0x51 offset=0 offset_len=1 Core: 209 devices, 27 uclasses, devicetree: separate ... u-boot=> i2c dev 0 && i2c md 0x51 0 50 Setting bus to 0 i2c - I2C sub-system
Usage: i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info ...
So the chip I noticed this issue with is 0x51 which an atmel,24c02 compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of (i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above i2c1-0x51 (i2c1=i2c@30a20000) is accessed early as it holds the board model information and at that point in time it's accessed with alen=1 (which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but by the time the 'i2c md 0x51 0 50' comes around i2c_get_chip_offset_len() returns 0 for this device. The other eeprom devices on i2c1 are never accessed by a driver so they would never have a 'default' alen set.
OK I see,
Where is the initial setting of alen supposed to have come?
The "u-boot,i2c-offset-len" property in the device tree.
Simon,
I see what happened here. The issue is caused by commit 8f8c04bf1ebbd ("i2c: fix stack buffer overflow vulnerability in i2c md command") which changed alen from int to uint (yet its still getting set to DEFAULT_ADDR_LEN which is -1) thus the 'if (alen > 3)' now returns CMD_RET_USAGE.
I'm not sure what the best fix is at this point - revert all or part of 8f8c04bf1ebbd
Nicolas, what is your opinion? To summarize commit 8f8c04bf1ebbd broke the ability to pass a -1 default address length to do_i2c_* such that something as common as 'i2c md 0x50 0 50' fails with a usage error.
Ah, ok. Well first we should add a test to dm/test/i2c.c to cover they failure you are seeing.
Yes, revert part of it and then add checks for -ve values?
Regards, Simon
I missed that -1 was a valid value for alen, as the checks "if (alen > 3) return CMD_RET_USAGE;" seemed to suppose that alen was non-negative (for example in do_i2c_read, https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/cmd/i2c.c#L271 and in do_i2c_write, do_i2c_md and do_i2c_md). The thing is, using signed types to hold sizes can lead to vulnerabilities, such as the one I fixed in commit 8f8c04bf1ebbd ("i2c: fix stack buffer overflow vulnerability in i2c md command"), where this computation did unexpected things when nbytes was negative:
linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes;
But it seems that some i2c drivers expect negative alen values (and not just -1). For example https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/drivers/i2c/mxc_i2c... documents "if alen < 0...". I agree to revert the parts of commit 8f8c04bf1ebbd which changed alen to unsigned int. Also, the comment of function get_alen (https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/cmd/i2c.c#L201 ) should probably be updated to document that it can also return negative values (and that it does not mean that an error occured).
By the way, how do you test commands? https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/test/dm/i2c.c seems to only test functions, and while testing my previous patch (commit 8f8c04bf1ebbd), I had some trouble finding a QEMU configuration with i2c devices.
Best regards, Nicolas Iooss
PS: I am not using my corporate email box to send emails as it appends a useless confidentiality note to my messages, without my control. Please keep nicolas.iooss+uboot@ledger.fr in the To/Cc list so that I can view replies.
Hi Nicolas,
On Wed, 17 Aug 2022 at 13:50, Nicolas IOOSS nicolas.iooss.ledger2022-08@proton.me wrote:
Hello all,
On Tue, Aug 16, 2022 at 1:47 PM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Tue, 16 Aug 2022 at 13:50, Tim Harvey tharvey@gateworks.com wrote:
On Mon, Aug 15, 2022 at 3:16 PM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Mon, 15 Aug 2022 at 11:48, Tim Harvey tharvey@gateworks.com wrote:
On Sat, Aug 13, 2022 at 7:59 AM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote: > > According to the comment block "The default {addr} parameter is one byte > (.1) which works well for memories and registers with 8 bits of address > space." > > While this is true for legacy I2C a default length of -1 is being passed > for DM_I2C which results in a usage error. > > Restore the documented behavior by always using a default alen of 1. > > Signed-off-by: Tim Harvey tharvey@gateworks.com > > This is an RFC as I'm unclear if we want to restore the legacy usage or > enforce a new usage (in which case the comment block should be updated) > and I'm not clear if this is documented in other places. If the decision > is to enforce a new usage then it is unclear to me how to specifiy the > default alen as there is no command for that (i2c alen [len]?). > --- > cmd/i2c.c | 10 ---------- > 1 file changed, 10 deletions(-) >
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
Simon,
Here's some annotated debug prints added where chip_offset is passed/set: Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit DRAM: 1 GiB i2c_chip_of_to_plat gsc@20 offset_len=1 i2c_chip_of_to_plat pmic@69 offset_len=1 i2c_get_chip i2c@30a20000 0x51 offset_len=1 i2c_bind_driver i2c@30a20000 offset_len=1 i2c_set_chip_offset_len generic_51 offset_len=1 dm_i2c_read generic_51 offset=0 offset_len=1 i2c_setup_offset 0x51 offset=0 offset_len=1 Core: 209 devices, 27 uclasses, devicetree: separate ... u-boot=> i2c dev 0 && i2c md 0x51 0 50 Setting bus to 0 i2c - I2C sub-system
Usage: i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info ...
So the chip I noticed this issue with is 0x51 which an atmel,24c02 compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of (i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above i2c1-0x51 (i2c1=i2c@30a20000) is accessed early as it holds the board model information and at that point in time it's accessed with alen=1 (which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but by the time the 'i2c md 0x51 0 50' comes around i2c_get_chip_offset_len() returns 0 for this device. The other eeprom devices on i2c1 are never accessed by a driver so they would never have a 'default' alen set.
OK I see,
Where is the initial setting of alen supposed to have come?
The "u-boot,i2c-offset-len" property in the device tree.
Simon,
I see what happened here. The issue is caused by commit 8f8c04bf1ebbd ("i2c: fix stack buffer overflow vulnerability in i2c md command") which changed alen from int to uint (yet its still getting set to DEFAULT_ADDR_LEN which is -1) thus the 'if (alen > 3)' now returns CMD_RET_USAGE.
I'm not sure what the best fix is at this point - revert all or part of 8f8c04bf1ebbd
Nicolas, what is your opinion? To summarize commit 8f8c04bf1ebbd broke the ability to pass a -1 default address length to do_i2c_* such that something as common as 'i2c md 0x50 0 50' fails with a usage error.
Ah, ok. Well first we should add a test to dm/test/i2c.c to cover they failure you are seeing.
Yes, revert part of it and then add checks for -ve values?
Regards, Simon
I missed that -1 was a valid value for alen, as the checks "if (alen > 3) return CMD_RET_USAGE;" seemed to suppose that alen was non-negative (for example in do_i2c_read, https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/cmd/i2c.c#L271 and in do_i2c_write, do_i2c_md and do_i2c_md). The thing is, using signed types to hold sizes can lead to vulnerabilities, such as the one I fixed in commit 8f8c04bf1ebbd ("i2c: fix stack buffer overflow vulnerability in i2c md command"), where this computation did unexpected things when nbytes was negative:
linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes;But it seems that some i2c drivers expect negative alen values (and not just -1). For example https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/drivers/i2c/mxc_i2c... documents "if alen < 0...".
I don't know what is going on there. The value is only -1 in cmd/ to indicate that it should not change.
I agree to revert the parts of commit 8f8c04bf1ebbd which changed alen to unsigned int. Also, the comment of function get_alen (https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/cmd/i2c.c#L201 ) should probably be updated to document that it can also return negative values (and that it does not mean that an error occured).
Yes
By the way, how do you test commands? https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-rc2/test/dm/i2c.c seems to only test functions, and while testing my previous patch (commit 8f8c04bf1ebbd), I had some trouble finding a QEMU configuration with i2c devices.
See test/dm/acpi.c for an example. There are existing i2c tests in test/dm/i2c.c so we can add to them. This does not use QEMU, but sandbox, a U-Boot build that runs on a host and in CI.
Best regards, Nicolas Iooss
PS: I am not using my corporate email box to send emails as it appends a useless confidentiality note to my messages, without my control. Please keep nicolas.iooss+uboot@ledger.fr in the To/Cc list so that I can view replies.
OK
Regards, Simon
participants (2)
-
Nicolas IOOSS -
Simon Glass