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 wrote:
>
> Hi Tim,
>
> On Tue, 16 Aug 2022 at 13:50, Tim Harvey wrote:
> >
> > On Mon, Aug 15, 2022 at 3:16 PM Simon Glass wrote:
> > >
> > > Hi Tim,
> > >
> > > On Mon, 15 Aug 2022 at 11:48, Tim Harvey wrote:
> > > >
> > > > On Sat, Aug 13, 2022 at 7:59 AM Simon Glass wrote:
> > > > >
> > > > > Hi Tim,
> > > > >
> > > > > On Thu, 11 Aug 2022 at 11:57, Tim Harvey
> > > > > 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
> > > > > >
> > > > > > 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@30a2 0x51 offset_len=1
> > > > i2c_bind_driver i2c@30a2 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@30a2) 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 comm
Re: Fwd: [PATCH] [RFC] cmd: i2c: fix default address len for DM_I2C
Hi Nicolas,
On Wed, 17 Aug 2022 at 13:50, Nicolas IOOSS
wrote:
>
> Hello all,
>
> On Tue, Aug 16, 2022 at 1:47 PM Simon Glass wrote:
> >
> > Hi Tim,
> >
> > On Tue, 16 Aug 2022 at 13:50, Tim Harvey wrote:
> > >
> > > On Mon, Aug 15, 2022 at 3:16 PM Simon Glass wrote:
> > > >
> > > > Hi Tim,
> > > >
> > > > On Mon, 15 Aug 2022 at 11:48, Tim Harvey wrote:
> > > > >
> > > > > On Sat, Aug 13, 2022 at 7:59 AM Simon Glass wrote:
> > > > > >
> > > > > > Hi Tim,
> > > > > >
> > > > > > On Thu, 11 Aug 2022 at 11:57, Tim Harvey
> > > > > > 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
> > > > > > >
> > > > > > > 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@30a2 0x51 offset_len=1
> > > > > i2c_bind_driver i2c@30a2 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@30a2) 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
