linux-kernel - [PATCH v3 03/12] HID: ft260: support i2c writes larger than HID report size

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: <20221030203403.4637-4-michael.zaidman@gmail.com>
Date:   Sun, 30 Oct 2022 22:33:54 +0200
From:   Michael Zaidman <michael.zaidman@...il.com>
To:     jikos@...nel.org
Cc:     linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        linux-i2c@...r.kernel.org, germain.hebert@...abb.com,
        Enrik.Berkhan@...a.de, Michael Zaidman <michael.zaidman@...il.com>,
        Guillaume Champagne <champagne.guillaume.c@...il.com>
Subject: [PATCH v3 03/12] HID: ft260: support i2c writes larger than HID report size

To support longer than one HID report size write, the driver splits a
single i2c message data payload into multiple i2c messages of HID report
size. However, it does not replicate the offset bytes within the EEPROM
chip in every consequent HID report because it is not and should not be
aware of the EEPROM type. It breaks the i2c write message integrity and
causes the EEPROM device not to acknowledge the second HID report keeping
the i2c bus busy until the ft260 controller reports failure.

This patch preserves the i2c write message integrity by manipulating the
i2c flag bits across multiple HID reports to be seen by the EEPROM device
as a single i2c write transfer.

Before:

$ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
Error: Sending messages failed: Input/output error

[  +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
[  +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
[  +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
[  +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
[  +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
[  +0.000241] ft260_i2c_reset: done
[  +0.000003] ft260_i2c_write: failed to start transfer, ret -5

After:

$ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Fill block with increment via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  71260           86             256           2           128

Signed-off-by: Michael Zaidman <michael.zaidman@...il.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@...il.com>
---
 drivers/hid/hid-ft260.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 44106cadd746..cec83f69ebdc 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -378,41 +378,46 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
 }
 
 static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
-			   int data_len, u8 flag)
+			   int len, u8 flag)
 {
-	int len, ret, idx = 0;
+	int ret, wr_len, idx = 0;
 	struct hid_device *hdev = dev->hdev;
 	struct ft260_i2c_write_request_report *rep =
 		(struct ft260_i2c_write_request_report *)dev->write_buf;
 
+	rep->flag = FT260_FLAG_START;
+
 	do {
-		if (data_len <= FT260_WR_DATA_MAX)
-			len = data_len;
-		else
-			len = FT260_WR_DATA_MAX;
+		if (len <= FT260_WR_DATA_MAX) {
+			wr_len = len;
+			if (flag == FT260_FLAG_START_STOP)
+				rep->flag |= FT260_FLAG_STOP;
+		} else {
+			wr_len = FT260_WR_DATA_MAX;
+		}
 
-		rep->report = FT260_I2C_DATA_REPORT_ID(len);
+		rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
 		rep->address = addr;
-		rep->length = len;
-		rep->flag = flag;
+		rep->length = wr_len;
 
-		memcpy(rep->data, &data[idx], len);
+		memcpy(rep->data, &data[idx], wr_len);
 
-		ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n",
-			  rep->report, addr, idx, len, data[0]);
+		ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n",
+			  rep->report, addr, idx, len, wr_len,
+			  rep->flag, data[0]);
 
 		ret = ft260_hid_output_report_check_status(dev, (u8 *)rep,
-							   len + 4);
+							   wr_len + 4);
 		if (ret < 0) {
-			hid_err(hdev, "%s: failed to start transfer, ret %d\n",
-				__func__, ret);
+			hid_err(hdev, "%s: failed with %d\n", __func__, ret);
 			return ret;
 		}
 
-		data_len -= len;
-		idx += len;
+		len -= wr_len;
+		idx += wr_len;
+		rep->flag = 0;
 
-	} while (data_len > 0);
+	} while (len > 0);
 
 	return 0;
 }
-- 
2.34.1