linux-kernel - Re: [External] Re: [PATCH] i2c: slave-eeprom: add latch mode

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] [day] [month] [year] [list]

Message-ID: <CA+J-oUvJ39Sz6Yt-8N33vJU=W0K8nN1oBwGwEDyHbhh+=BZpRQ@mail.gmail.com>
Date: Tue, 20 May 2025 11:01:04 +0800
From: Zhang Jian <zhangjian.3032@...edance.com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	Jian Zhang <zhangjian.3032@...edance.com>, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] i2c: slave-eeprom: add latch mode

On Tue, May 20, 2025 at 1:03 AM Wolfram Sang
<wsa+renesas@...g-engineering.com> wrote:
>
> On Mon, Dec 09, 2024 at 02:04:21PM +0800, Jian Zhang wrote:
> > The read operation is locked by byte, while the write operation is
> > locked by block (or based on the amount of data written). If we need to
> > ensure the integrity of a "block" of data that the other end can read,
> > then we need a latch mode, lock the buffer when a read operation is
> > requested.
>
> I don't really understand what you want to fix here. Does this patch
> really fix your issue because...

The scenario I’m dealing with:
* I’m using this driver to simulate an EEPROM device.
* One byte of the EEPROM content contains the CRC of the preceding data.
* Each time I update the EEPROM data, I use i2c_slave_eeprom_bin_write
to write the entire buffer, so the data in memory is always
consistent.
* I expect the I2C master (peer) to be able to perform a block read
and get the full, correct data including a valid CRC.

The problem I’ve encountered:
* In i2c_slave_eeprom_slave_cb, a block read from the master triggers
multiple callbacks, each returning one byte.
 This results in a sequence like:
 1. Master sends a write
 2. Master reads the first byte.
 3. The EEPROM buffer is updated.
 4, Master reads the second byte.

This may lead to a mismatch during a single block read,
where the master receives data that is partially from the,
 old buffer and partially from the new one—causing
CRC validation to fail.

>
> >       switch (event) {
> >       case I2C_SLAVE_WRITE_RECEIVED:
> > +             if (eeprom->latch) {
> > +                     spin_lock(&eeprom->buffer_lock);
> > +                     memcpy(eeprom->buffer_latch, eeprom->buffer, eeprom->bin.size);
> > +                     spin_unlock(&eeprom->buffer_lock);
> > +             }
>
> ... what advantage brings you this memcpy of the buffer to a latch after
> every single byte is received?

If you agree that the scenario I described earlier is a valid case
worth considering,
I make a copy of the buffer at the beginning of a write operation,
and then use this latched buffer for all subsequent reads until the
STOP condition.

Sorry, I think I placed the copy logic in the wrong spot earlier.
Perhaps it would be more appropriate to do it in the
I2C_SLAVE_WRITE_REQUESTED callback.

>
> > +     if (of_property_read_bool(client->adapter->dev.of_node, "use-latch")) {
>
> If there really is a problem, we don't need a binding for it but should
> use the fix in all cases.
>
i got it, thanks.