From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 630CDC25B08 for ; Wed, 17 Aug 2022 22:45:00 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 35B2484A43; Thu, 18 Aug 2022 00:44:58 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="VyxJr1wV"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5E33084A54; Thu, 18 Aug 2022 00:44:56 +0200 (CEST) Received: from mail-yw1-x112c.google.com (mail-yw1-x112c.google.com [IPv6:2607:f8b0:4864:20::112c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BF6DF848AF for ; Thu, 18 Aug 2022 00:44:52 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-yw1-x112c.google.com with SMTP id 00721157ae682-324ec5a9e97so269219727b3.7 for ; Wed, 17 Aug 2022 15:44:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc; bh=/45W9bYFVYbtv54V25746jOhhtJmyuCYg3kp+3vkHlY=; b=VyxJr1wVObBpv9bRsDmwsF9GZKpVrzl1a6yMmQjYe7QlOpWynYDfwvAoR6oIsMdQn1 tm/PTzBcNTTCL93wi0CvqimXZ2Nq+16Sy/hbJ1XwapGpMIfjIsCxzgN/z/3mT90t06RI atJ6QVdruZufRrFTPVlvOzoZNRDdWEVTIeO1U= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc; bh=/45W9bYFVYbtv54V25746jOhhtJmyuCYg3kp+3vkHlY=; b=0swMXe1L8EPAMZoPflfAk3VPDNKnOVReWv0xKNYWjAnRDhu5xufUwEZX/QtCDHCqcg XuPqirVfjpeZeyRgoPJ++MWufgJn6B0QaXgBcYnvizQrbeeLjN6REo9dbQQREEKPoPkE YTxAUSzsQhx8b1yakBkko9qcLl0GvWDPrSNF8D79/9Ts8SUWX3qSwAT0TXWqQaDATIav hfLlj+QN5yLrgUU/l903ortvv7xiP7D29/+BGX68W9ne8TI1yhoPb53Zke/n2FXBe4KB 4SKyakJEzZLkYlppBB6OPvBQSQtpZfZZgaRU2F1eDLFUzz8kBklL+NgSRjVDQqYR+0PN sfvQ== X-Gm-Message-State: ACgBeo22kUBaoU1RL9LLmKFQX+CrdU2ub0QJw7I5V1j04jFhnDpfJb9O x2BBw7ril+Ke71P6pE/9BUKa9eX2Tj7km3/Nf8u4UZKy5NbQQg== X-Google-Smtp-Source: AA6agR5klAbiczToGWWADFTnSRnA8w67Oy6pJ0Amn5sd5ib5R1vP5LTrIfHaaEDcWF4p9y5LCvxDuztKFbZJPV2z1wo= X-Received: by 2002:a81:7bd6:0:b0:328:297a:f31f with SMTP id w205-20020a817bd6000000b00328297af31fmr302317ywc.469.1660776290863; Wed, 17 Aug 2022 15:44:50 -0700 (PDT) MIME-Version: 1.0 References: <20220811175740.3984704-1-tharvey@gateworks.com> In-Reply-To: From: Simon Glass Date: Wed, 17 Aug 2022 16:44:37 -0600 Message-ID: Subject: Re: Fwd: [PATCH] [RFC] cmd: i2c: fix default address len for DM_I2C To: Nicolas IOOSS Cc: Tim Harvey , "nicolas.iooss+uboot@ledger.fr" , U-Boot Mailing List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 wr= ote: > > > > > > > > > > On Sat, Aug 13, 2022 at 7:59 AM Simon Glass wr= ote: > > > > > > > > > > > > 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 b= eing passed > > > > > > > for DM_I2C which results in a usage error. > > > > > > > > > > > > > > Restore the documented behavior by always using a default ale= n of 1. > > > > > > > > > > > > > > Signed-off-by: Tim Harvey > > > > > > > > > > > > > > This is an RFC as I'm unclear if we want to restore the legac= y usage or > > > > > > > enforce a new usage (in which case the comment block should b= e updated) > > > > > > > and I'm not clear if this is documented in other places. If t= he decision > > > > > > > is to enforce a new usage then it is unclear to me how to spe= cifiy 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 b= y the > > > > > > peripheral device and we don't want to overwrite it: > > > > > > > > > > > > if (!ret && alen !=3D -1) > > > > > > ret =3D i2c_set_chip_offset_len(dev, alen); > > > > > > > > > > > > > > > > Simon, > > > > > > > > > > Here's some annotated debug prints added where chip_offset is pas= sed/set: > > > > > Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit > > > > > DRAM: 1 GiB > > > > > i2c_chip_of_to_plat gsc@20 offset_len=3D1 > > > > > i2c_chip_of_to_plat pmic@69 offset_len=3D1 > > > > > i2c_get_chip i2c@30a20000 0x51 offset_len=3D1 > > > > > i2c_bind_driver i2c@30a20000 offset_len=3D1 > > > > > i2c_set_chip_offset_len generic_51 offset_len=3D1 > > > > > dm_i2c_read generic_51 offset=3D0 offset_len=3D1 > > > > > i2c_setup_offset 0x51 offset=3D0 offset_len=3D1 > > > > > Core: 209 devices, 27 uclasses, devicetree: separate > > > > > ... > > > > > u-boot=3D> 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,24c0= 2 > > > > > 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=3Di2c@30a20000) is accessed early as it holds the= board > > > > > model information and at that point in time it's accessed with al= en=3D1 > > > > > (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 ee= prom > > > > > devices on i2c1 are never accessed by a driver so they would neve= r > > > > > 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 brok= e > > > 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 e= xample 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 thin= g 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 =3D (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes; > > But it seems that some i2c drivers expect negative alen values (and not j= ust -1). For example https://source.denx.de/u-boot/u-boot/-/blob/v2022.10-r= c2/drivers/i2c/mxc_i2c.c#L640 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-boo= t/-/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 findi= ng 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 kee= p nicolas.iooss+uboot@ledger.fr in the To/Cc list so that I can view replie= s. OK Regards, Simon