linux-kernel - Re: [PATCH 4/5] Input: pinephone-keyboard

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: <20220130030031.v2jbebaz7rs2d6vm@core.my.home>
Date:   Sun, 30 Jan 2022 04:00:31 +0100
From:   Ondřej Jirman <x@....cz>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-i2c@...r.kernel.org, Wolfram Sang <wsa@...nel.org>
Subject: Re: [PATCH 4/5] Input: pinephone-keyboard - Support the proxied I2C
 bus

On Sat, Jan 29, 2022 at 08:43:30PM -0600, Samuel Holland wrote:
> On 1/29/22 8:05 PM, Ondřej Jirman wrote:
> > 
> > Please use a single read transfer to get both command result and data.
> > There will be less risk that some userspace app will issue another command
> > in between command status being read as 0 and data byte being read.
> > 
> > Otherwise if you use this in some read/modify/write operation, you
> > may write unexpected value to PMIC. I2C register layout is designed
> > to make this as optimal as possible in a single I2C transaction, so
> > you only need 3 bytes to start command and 2 bytes to read the result
> > and data, both in a single xfer. There's very high likelihood the command
> > will complete in those 300 - 500 us anyway, because the timing is
> > predictable. If this delay is set right, it's almost guaranteed,
> > only two xfers will be necessary to run the command and get the result+
> > status.
> 
> I did this originally, but it causes a different race condition: since the data
> is read first, the command can complete between when the data is read and when
> the result is read. If this happens, the command will be seen as complete, but
> the data will be garbage.
> 
> This caused occasional read errors for the charger's power supply properties,
> because I2C reads sometimes returned nonsensical values for those bytes.

Oh, well. :) I guess the firmware would need to wait for any ongoing I2C
tranfer to finish before setting the command status to 0, for this to work.
Another lesson learned. :(

> > And if possible, it would be best if the bus was somehow made busy for
> > other users, until the whole comand/result sequence completes, to eliminate
> > the possibility of another command being issued by other bus users
> > around [1].
> 
> Yes, I can add a call to i2c_lock_bus() here.

Perfect.

thank you,
	o.

> Regards,
> Samuel