Off-by-one error in CP2112 HID driver affecting SMBus block read output

Affected file: drivers/hid/hid-cp2112.c
Kernel version: v5.15.195 (verified) - master (suspected) Tool used: i2cget -y 8 0xb 0x78 s <-- reading a battery gauge chip

Observed behavior:
Incorrect trailing last byte in 32 bytes SMBus block read response.

Expected output:
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xaa 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x55

Actual SMBus block data read output:
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xaa 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x00

I2C block data read output (from i2cget -y 8 0xb 0x78 i):
0x20 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xaa 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff

Description:
When performing an SMBus block read using the CP2112 HID USB-to-SMBus bridge with the i2cget utility, the last byte of the response for a 32 bytes block read is, in this case, always 0x00 instead of the actual value. In this sample case shown above the actual last byte is 0x55 but the driver returns 0x00 (uninitialized value).
This happens consistently for any 32 bytes block read on any register. Shorter than 32 bytes block transfers are unaffected.

Suspected cause:
For what I could understand checking the source code this seems an off-by-one error in the read request and buffer handling, limited by the I2C_SMBUS_BLOCK_MAX 32 bytes read value and the response parsing logic within the CP2112 driver since that with a custom hacked module (see patch below) I could read that last byte.

Steps to reproduce:
1. Connect a CP2112 device and a proper slave device to read from.
2. Use i2cget to perform a known 32 bytes block read with this device:
i.e: i2cget -y 8 0xb 0x78 s
3. Compare the output against the expected data from the target SMBus device. The last byte probably could appear as any other uninitialized buffer value, I guess.
From 'i2cget -y 8 0xb 0x78 i' the initial 0x20 is the block length and the last 32th byte is completely missing.

Impact:
This bug produces incorrect output and will lead to data corruption or data misinterpretation when reading from SMBus devices using this driver.

Suggested fix:
Review the buffer handling and read length in the CP2112 driver SMBus and I2C block read and write implementations.
I have not checked if write operations are affected, but probably would be a good idea to verify that the 32 bytes block transfers are handled properly.

Additional info:
The issue is reproducible with different CP2112 devices (silicon rev F04) and v5.15.x stock kernel builds. Probably also affects newer versions since for what I could understand, despite the additional checking changes added to the driver the logic seems basically the same.

Patch: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/plain/drivers/hid/hid-cp2112.c?h=v5.15.195

@@ -703,7 +703,7 @@
 	case I2C_SMBUS_BLOCK_DATA:
 		if (I2C_SMBUS_READ == read_write) {
 			count = cp2112_write_read_req(buf, addr,
-						      I2C_SMBUS_BLOCK_MAX,
+						      I2C_SMBUS_BLOCK_MAX + 1,
 						      command, NULL, 0);
 		} else {
 			count = cp2112_write_req(buf, addr, command,

@@ -796,7 +796,7 @@
 		memcpy(data->block + 1, buf, read_length);
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		if (read_length > I2C_SMBUS_BLOCK_MAX) {
+		if (read_length > I2C_SMBUS_BLOCK_MAX + 1) {
 			ret = -EPROTO;
 			goto power_normal;
 		}

--

Just following up on this bug report and proposed fix. I would appreciate feedback on whether this approach is acceptable or if there is a preferred way to handle this quirk.

After thoroughly reviewing the driver code and rereading AN495 I still don't know why these chips (tried 5 of them so far) are showing the previously stated observed behaviour, because the driver logic seems compliant with the AN495 CP2112 interface specification.

But performing an SMBus block read using the CP2112 HID USB-to-SMBus bridge with these chips the last byte of a 32-byte response is consistently incorrect unless an extra byte was requested.

A flag could be added at driver load time to enable a quirk fix for affected chips but the end user first would need to notice or know that their adapter is affected.

I have not been able to figure out a way to detect those affected chips that will produce corrupted output without notice or warning other than directly checking the serial number.

So have written a quirks patch against v5.15.195 that applies to a given serial number range or device version and can be enabled for those known to be affected with minimal impact on the rest.

Alternatively I think that at least some sort of warning notice always should be included.

@@ -26,6 +26,14 @@
 #include <linux/nls.h>
 #include <linux/usb/ch9.h>
 #include "hid-ids.h"
+#include <linux/usb.h>
+
+#ifndef CP2112_QUIRKS
+#define CP2112_SN_RANGE_QUIRKS_START	"00670000"
+#define CP2112_SN_RANGE_QUIRKS_END	"00670FFF"
+#define CP2112_DEVICE_VERSION_QUIRK	2
+#define CP2112_QUIRK_SMBUS_BLOCK33 BIT(0)
+#endif
 
 #define CP2112_REPORT_MAX_LENGTH		64
 #define CP2112_GPIO_CONFIG_LENGTH		5

@@ -170,6 +178,7 @@
 	struct delayed_work gpio_poll_worker;
 	unsigned long irq_mask;
 	u8 gpio_prev_state;
+	unsigned long quirks;
 };
 
 static int gpio_push_pull = 0xFF;

@@ -642,6 +651,7 @@
 	__le16 word;
 	ssize_t count;
 	size_t read_length = 0;
+	size_t max_length;
 	unsigned int retries;
 	int ret;
 

@@ -702,8 +712,14 @@
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
 		if (I2C_SMBUS_READ == read_write) {
+			read_length = I2C_SMBUS_BLOCK_MAX ;
+			/* Apply quirk: allow one extra byte for CP2112 block read */
+			if (dev->quirks & CP2112_QUIRK_SMBUS_BLOCK33) {
+				read_length += 1;
+				hid_info(hdev, "Quirk: using block read size %lu\n", read_length);
+			}
 			count = cp2112_write_read_req(buf, addr,
-						      I2C_SMBUS_BLOCK_MAX,
+						      read_length,
 						      command, NULL, 0);
 		} else {
 			count = cp2112_write_req(buf, addr, command,

@@ -796,7 +812,14 @@
 		memcpy(data->block + 1, buf, read_length);
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		if (read_length > I2C_SMBUS_BLOCK_MAX) {
+		max_length = I2C_SMBUS_BLOCK_MAX;
+
+		if (dev->quirks & CP2112_QUIRK_SMBUS_BLOCK33) {
+			max_length += 1;
+			hid_info(hdev, "Quirk: allowing block read up to %lu bytes\n", max_length);
+		}
+
+		if (read_length > max_length) {
 			ret = -EPROTO;
 			goto power_normal;
 		}

@@ -1235,6 +1258,9 @@
 	struct gpio_irq_chip *girq;
 	int ret;
 
+	struct usb_device *udev;
+	char serial[64] = {0};
+
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;

@@ -1282,6 +1308,26 @@
 	hid_info(hdev, "Part Number: 0x%02X Device Version: 0x%02X\n",
 		 buf[1], buf[2]);
 
+	if (buf[2] == CP2112_DEVICE_VERSION_QUIRK) {
+		dev->quirks |= CP2112_QUIRK_SMBUS_BLOCK33;
+		hid_info(hdev, "Enabled SMBus block+1 quirk for device version 0x%02X\n", buf[2]);
+	}
+
+	udev = interface_to_usbdev(to_usb_interface(hdev->dev.parent));
+
+	if (udev->descriptor.iSerialNumber) {
+		ret = usb_string(udev, udev->descriptor.iSerialNumber, serial, sizeof(serial));
+		if (ret > 0)
+			hid_info(hdev, "CP2112 device serial number: %s\n", serial);
+	}
+
+	if (strlen(serial) == 8 &&
+		strncmp(serial, CP2112_SN_RANGE_QUIRKS_START, 8) >= 0 &&
+		strncmp(serial, CP2112_SN_RANGE_QUIRKS_END, 8) <= 0) {
+			dev->quirks |= CP2112_QUIRK_SMBUS_BLOCK33;
+			hid_info(hdev, "Enabled SMBus block+1 quirk for serial %s\n", serial);
+	}
+
 	ret = cp2112_hid_get(hdev, CP2112_SMBUS_CONFIG, (u8 *)&config,
 			     sizeof(config), HID_FEATURE_REPORT);
 	if (ret != sizeof(config)) {

--