[RESEND PATCH v1 0/4] Fixes for the mcp2221 HID-to-I2C-bridge driver

[RESEND PATCH v1 0/4] Fixes for the mcp2221 HID-to-I2C-bridge driver

[Enrik Berkhan]

In this patch series are fixes for issues I found during recent tests of
an MCP2221 board.

- you can confuse the kernel driver when using the chip from user mode
  via /dev/hidrawX, typically leading to a NULL pointer dereference in
  the driver's HID raw event handler

- the driver needs > 15s to initialize because the HID raw handling is
  not enabled during initialization of the GPIO part

- data shared with the bottom half code is not protected from concurrent
  access

- the rxbuf pointer can become invalid or even stale if the device would
  send unsolicited reports

Enrik Berkhan (4):
  HID: mcp2221: don't connect hidraw
  HID: mcp2221: enable HID I/O during GPIO probe
  HID: mcp2221: protect shared data with spin lock
  HID: mcp2221: avoid stale rxbuf pointer

 drivers/hid/hid-mcp2221.c | 114 ++++++++++++++++++++++++++++++++------
 1 file changed, 97 insertions(+), 17 deletions(-)
---

Resend because I had a typo in the linux-input mailing list address.
Also adding linux-kernel to increase visibility.

-- 
2.34.1

[PATCH v1 1/4] HID: mcp2221: don't connect hidraw

[Enrik Berkhan]

The MCP2221 driver should not connect to the hidraw userspace interface,
as it needs exclusive access to the chip.

If you want to use /dev/hidrawX with the MCP2221, you need to avoid
binding this driver to the device and use the hid generic driver instead
(e.g. using udev rules).

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index de52e9f7bb8c..0ca2a7b96825 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -840,12 +840,17 @@ static int mcp2221_probe(struct hid_device *hdev,
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	/* This driver uses the .raw_event callback and therefore does not need any
+	 * HID_CONNECT_xxx flags. */
+	ret = hid_hw_start(hdev, 0);
 	if (ret) {
 		hid_err(hdev, "can't start hardware\n");
 		return ret;
 	}
 
+	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+			hdev->version & 0xff, hdev->name, hdev->phys);
+
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "can't open device\n");
@@ -870,8 +875,7 @@ static int mcp2221_probe(struct hid_device *hdev,
 	mcp->adapter.retries = 1;
 	mcp->adapter.dev.parent = &hdev->dev;
 	snprintf(mcp->adapter.name, sizeof(mcp->adapter.name),
-			"MCP2221 usb-i2c bridge on hidraw%d",
-			((struct hidraw *)hdev->hidraw)->minor);
+			"MCP2221 usb-i2c bridge");
 
 	ret = i2c_add_adapter(&mcp->adapter);
 	if (ret) {
-- 
2.34.1

[PATCH v1 2/4] HID: mcp2221: enable HID I/O during GPIO probe

[Enrik Berkhan]

As soon as the GPIO driver part will be enabled in mcp2221_probe(), the
first HID reports will be exchanged with the chip because the GPIO
driver immediately calls mcp_gpio_get_direction(). HID I/O has to be
enabled explicitly during mcp2221_probe() to receive response reports.

Otherwise, all four mcp_gpio_get_direction() calls will run into the
four second timeout of mcp_send_report(), which will block the driver
for about 16s during startup.

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 0ca2a7b96825..5d8898f3f2e3 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -902,6 +902,9 @@ static int mcp2221_probe(struct hid_device *hdev,
 	mcp->gc->can_sleep = 1;
 	mcp->gc->parent = &hdev->dev;
 
+	/* Enable reception of HID reports during GPIO initialization */
+	hid_device_io_start(hdev);
+
 	ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
 	if (ret)
 		goto err_gc;
-- 
2.34.1

[PATCH v1 3/4] HID: mcp2221: protect shared data with spin lock

[Enrik Berkhan]

Various fields of the driver's per instance data are read and written
both from process context during i2c or gpio processing and the HID
.raw_event callback. The .raw_event callback usually runs in softirq
context. Concurrent access to the shared fields is protected with
spin_{un}lock_bh().

Note: the higher level mutex to prevent user space calls from running
concurrently is still needed. The spin lock only addresses low level
consistency of eg. tx buffer contents and length.

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 61 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 5d8898f3f2e3..d17839e09ebc 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/hid.h>
@@ -88,6 +89,7 @@ struct mcp2221 {
 	struct hid_device *hdev;
 	struct i2c_adapter adapter;
 	struct mutex lock;
+	spinlock_t raw_event_lock;
 	struct completion wait_in_report;
 	u8 *rxbuf;
 	u8 txbuf[64];
@@ -153,8 +155,10 @@ static int mcp_send_data_req_status(struct mcp2221 *mcp,
 /* Check pass/fail for actual communication with i2c slave */
 static int mcp_chk_last_cmd_status(struct mcp2221 *mcp)
 {
+	spin_lock_bh(&mcp->raw_event_lock);
 	memset(mcp->txbuf, 0, 8);
 	mcp->txbuf[0] = MCP2221_I2C_PARAM_OR_STATUS;
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	return mcp_send_data_req_status(mcp, mcp->txbuf, 8);
 }
@@ -162,9 +166,11 @@ static int mcp_chk_last_cmd_status(struct mcp2221 *mcp)
 /* Cancels last command releasing i2c bus just in case occupied */
 static int mcp_cancel_last_cmd(struct mcp2221 *mcp)
 {
+	spin_lock_bh(&mcp->raw_event_lock);
 	memset(mcp->txbuf, 0, 8);
 	mcp->txbuf[0] = MCP2221_I2C_PARAM_OR_STATUS;
 	mcp->txbuf[2] = MCP2221_I2C_CANCEL;
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	return mcp_send_data_req_status(mcp, mcp->txbuf, 8);
 }
@@ -173,10 +179,12 @@ static int mcp_set_i2c_speed(struct mcp2221 *mcp)
 {
 	int ret;
 
+	spin_lock_bh(&mcp->raw_event_lock);
 	memset(mcp->txbuf, 0, 8);
 	mcp->txbuf[0] = MCP2221_I2C_PARAM_OR_STATUS;
 	mcp->txbuf[3] = MCP2221_I2C_SET_SPEED;
 	mcp->txbuf[4] = mcp->cur_i2c_clk_div;
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 8);
 	if (ret) {
@@ -209,12 +217,14 @@ static int mcp_i2c_write(struct mcp2221 *mcp,
 		len = 60;
 
 	do {
+		spin_lock_bh(&mcp->raw_event_lock);
 		mcp->txbuf[0] = type;
 		mcp->txbuf[1] = msg->len & 0xff;
 		mcp->txbuf[2] = msg->len >> 8;
 		mcp->txbuf[3] = (u8)(msg->addr << 1);
 
 		memcpy(&mcp->txbuf[4], &msg->buf[idx], len);
+		spin_unlock_bh(&mcp->raw_event_lock);
 
 		ret = mcp_send_data_req_status(mcp, mcp->txbuf, len + 4);
 		if (ret)
@@ -261,6 +271,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
 	int ret;
 	u16 total_len;
 
+	spin_lock_bh(&mcp->raw_event_lock);
 	mcp->txbuf[0] = type;
 	if (msg) {
 		mcp->txbuf[1] = msg->len & 0xff;
@@ -275,16 +286,21 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
 		total_len = smbus_len;
 		mcp->rxbuf = smbus_buf;
 	}
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);
 	if (ret)
 		return ret;
 
+	spin_lock_bh(&mcp->raw_event_lock);
 	mcp->rxbuf_idx = 0;
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	do {
+		spin_lock_bh(&mcp->raw_event_lock);
 		memset(mcp->txbuf, 0, 4);
 		mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
+		spin_unlock_bh(&mcp->raw_event_lock);
 
 		ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
 		if (ret)
@@ -365,6 +381,7 @@ static int mcp_smbus_write(struct mcp2221 *mcp, u16 addr,
 {
 	int data_len, ret;
 
+	spin_lock_bh(&mcp->raw_event_lock);
 	mcp->txbuf[0] = type;
 	mcp->txbuf[1] = len + 1; /* 1 is due to command byte itself */
 	mcp->txbuf[2] = 0;
@@ -391,6 +408,7 @@ static int mcp_smbus_write(struct mcp2221 *mcp, u16 addr,
 		memcpy(&mcp->txbuf[5], buf, len);
 		data_len = len + 5;
 	}
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, data_len);
 	if (ret)
@@ -479,9 +497,11 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 			if (ret)
 				goto exit;
 
+			spin_lock_bh(&mcp->raw_event_lock);
 			mcp->rxbuf_idx = 0;
 			mcp->rxbuf = data->block;
 			mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
+			spin_unlock_bh(&mcp->raw_event_lock);
 			ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
 			if (ret)
 				goto exit;
@@ -502,9 +522,11 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 			if (ret)
 				goto exit;
 
+			spin_lock_bh(&mcp->raw_event_lock);
 			mcp->rxbuf_idx = 0;
 			mcp->rxbuf = data->block;
 			mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
+			spin_unlock_bh(&mcp->raw_event_lock);
 			ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
 			if (ret)
 				goto exit;
@@ -573,12 +595,16 @@ static int mcp_gpio_get(struct gpio_chip *gc,
 	int ret;
 	struct mcp2221 *mcp = gpiochip_get_data(gc);
 
+	mutex_lock(&mcp->lock);
+
+	spin_lock_bh(&mcp->raw_event_lock);
 	mcp->txbuf[0] = MCP2221_GPIO_GET;
 
 	mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].value);
+	spin_unlock_bh(&mcp->raw_event_lock);
 
-	mutex_lock(&mcp->lock);
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
+
 	mutex_unlock(&mcp->lock);
 
 	return ret;
@@ -589,6 +615,9 @@ static void mcp_gpio_set(struct gpio_chip *gc,
 {
 	struct mcp2221 *mcp = gpiochip_get_data(gc);
 
+	mutex_lock(&mcp->lock);
+
+	spin_lock_bh(&mcp->raw_event_lock);
 	memset(mcp->txbuf, 0, 18);
 	mcp->txbuf[0] = MCP2221_GPIO_SET;
 
@@ -596,15 +625,17 @@ static void mcp_gpio_set(struct gpio_chip *gc,
 
 	mcp->txbuf[mcp->gp_idx - 1] = 1;
 	mcp->txbuf[mcp->gp_idx] = !!value;
+	spin_unlock_bh(&mcp->raw_event_lock);
 
-	mutex_lock(&mcp->lock);
 	mcp_send_data_req_status(mcp, mcp->txbuf, 18);
+
 	mutex_unlock(&mcp->lock);
 }
 
 static int mcp_gpio_dir_set(struct mcp2221 *mcp,
 				unsigned int offset, u8 val)
 {
+	spin_lock_bh(&mcp->raw_event_lock);
 	memset(mcp->txbuf, 0, 18);
 	mcp->txbuf[0] = MCP2221_GPIO_SET;
 
@@ -612,6 +643,7 @@ static int mcp_gpio_dir_set(struct mcp2221 *mcp,
 
 	mcp->txbuf[mcp->gp_idx - 1] = 1;
 	mcp->txbuf[mcp->gp_idx] = val;
+	spin_unlock_bh(&mcp->raw_event_lock);
 
 	return mcp_send_data_req_status(mcp, mcp->txbuf, 18);
 }
@@ -654,21 +686,31 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc,
 	int ret;
 	struct mcp2221 *mcp = gpiochip_get_data(gc);
 
+	mutex_lock(&mcp->lock);
+
+	spin_lock_bh(&mcp->raw_event_lock);
 	mcp->txbuf[0] = MCP2221_GPIO_GET;
 
 	mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].direction);
+	spin_unlock_bh(&mcp->raw_event_lock);
 
-	mutex_lock(&mcp->lock);
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
-	mutex_unlock(&mcp->lock);
+
+	spin_lock_bh(&mcp->raw_event_lock);
 
 	if (ret)
-		return ret;
+		goto out_unlock;
 
 	if (mcp->gpio_dir == MCP2221_DIR_IN)
-		return GPIO_LINE_DIRECTION_IN;
+		ret = GPIO_LINE_DIRECTION_IN;
+	else
+		ret = GPIO_LINE_DIRECTION_OUT;
 
-	return GPIO_LINE_DIRECTION_OUT;
+out_unlock:
+	spin_unlock_bh(&mcp->raw_event_lock);
+	mutex_unlock(&mcp->lock);
+
+	return ret;
 }
 
 /* Gives current state of i2c engine inside mcp2221 */
@@ -716,6 +758,8 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 	u8 *buf;
 	struct mcp2221 *mcp = hid_get_drvdata(hdev);
 
+	spin_lock_bh(&mcp->raw_event_lock);
+
 	switch (data[0]) {
 
 	case MCP2221_I2C_WR_DATA:
@@ -821,6 +865,8 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 		complete(&mcp->wait_in_report);
 	}
 
+	spin_unlock_bh(&mcp->raw_event_lock);
+
 	return 1;
 }
 
@@ -857,6 +903,7 @@ static int mcp2221_probe(struct hid_device *hdev,
 		goto err_hstop;
 	}
 
+	spin_lock_init(&mcp->raw_event_lock);
 	mutex_init(&mcp->lock);
 	init_completion(&mcp->wait_in_report);
 	hid_set_drvdata(hdev, mcp);
-- 
2.34.1

[PATCH v1 4/4] HID: mcp2221: avoid stale rxbuf pointer

[Enrik Berkhan]

In case the MCP2221 driver receives an unexpected read complete report
from the device, the data should not be copied to mcp->rxbuf. The
pointer might be NULL or even stale, having been set during an earlier
transaction.

Further, some bounds checking has been added.

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 44 +++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index d17839e09ebc..faccb3c03d33 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -94,6 +94,7 @@ struct mcp2221 {
 	u8 *rxbuf;
 	u8 txbuf[64];
 	int rxbuf_idx;
+	int rxbuf_len;
 	int status;
 	u8 cur_i2c_clk_div;
 	struct gpio_chip *gc;
@@ -286,15 +287,13 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
 		total_len = smbus_len;
 		mcp->rxbuf = smbus_buf;
 	}
+	mcp->rxbuf_len = total_len;
+	mcp->rxbuf_idx = 0;
 	spin_unlock_bh(&mcp->raw_event_lock);
 
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);
 	if (ret)
-		return ret;
-
-	spin_lock_bh(&mcp->raw_event_lock);
-	mcp->rxbuf_idx = 0;
-	spin_unlock_bh(&mcp->raw_event_lock);
+		goto out_invalidate_rxbuf;
 
 	do {
 		spin_lock_bh(&mcp->raw_event_lock);
@@ -304,15 +303,22 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
 
 		ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
 		if (ret)
-			return ret;
+			goto out_invalidate_rxbuf;
 
 		ret = mcp_chk_last_cmd_status(mcp);
 		if (ret)
-			return ret;
+			goto out_invalidate_rxbuf;
 
 		usleep_range(980, 1000);
 	} while (mcp->rxbuf_idx < total_len);
 
+out_invalidate_rxbuf:
+	spin_lock_bh(&mcp->raw_event_lock);
+	mcp->rxbuf = NULL;
+	mcp->rxbuf_len = 0;
+	mcp->rxbuf_idx = 0;
+	spin_unlock_bh(&mcp->raw_event_lock);
+
 	return ret;
 }
 
@@ -500,9 +506,15 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 			spin_lock_bh(&mcp->raw_event_lock);
 			mcp->rxbuf_idx = 0;
 			mcp->rxbuf = data->block;
+			mcp->rxbuf_len = sizeof(data->block);
 			mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
 			spin_unlock_bh(&mcp->raw_event_lock);
 			ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
+			spin_lock_bh(&mcp->raw_event_lock);
+			mcp->rxbuf_idx = 0;
+			mcp->rxbuf = NULL;
+			mcp->rxbuf_len = 0;
+			spin_unlock_bh(&mcp->raw_event_lock);
 			if (ret)
 				goto exit;
 		} else {
@@ -525,9 +537,15 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 			spin_lock_bh(&mcp->raw_event_lock);
 			mcp->rxbuf_idx = 0;
 			mcp->rxbuf = data->block;
+			mcp->rxbuf_len = sizeof(data->block);
 			mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
 			spin_unlock_bh(&mcp->raw_event_lock);
 			ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
+			spin_lock_bh(&mcp->raw_event_lock);
+			mcp->rxbuf_idx = 0;
+			mcp->rxbuf = NULL;
+			mcp->rxbuf_len = 0;
+			spin_unlock_bh(&mcp->raw_event_lock);
 			if (ret)
 				goto exit;
 		} else {
@@ -756,6 +774,7 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 				struct hid_report *report, u8 *data, int size)
 {
 	u8 *buf;
+	int len;
 	struct mcp2221 *mcp = hid_get_drvdata(hdev);
 
 	spin_lock_bh(&mcp->raw_event_lock);
@@ -813,9 +832,15 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 				break;
 			}
 			if (data[2] == MCP2221_I2C_READ_COMPL) {
+				if (mcp->rxbuf == NULL || mcp->rxbuf_idx >= mcp->rxbuf_len)
+					goto out; /* no complete() in this case */
+
 				buf = mcp->rxbuf;
-				memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
-				mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
+				len = data[3];
+				if (len > mcp->rxbuf_len - mcp->rxbuf_idx)
+					len = mcp->rxbuf_len - mcp->rxbuf_idx;
+				memcpy(&buf[mcp->rxbuf_idx], &data[4], len);
+				mcp->rxbuf_idx = mcp->rxbuf_idx + len;
 				mcp->status = 0;
 				break;
 			}
@@ -865,6 +890,7 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 		complete(&mcp->wait_in_report);
 	}
 
+out:
 	spin_unlock_bh(&mcp->raw_event_lock);
 
 	return 1;
-- 
2.34.1

[PATCH v2 0/3] Fixes for the mcp2221 HID-to-I2C-bridge driver

[Enrik Berkhan]

In this patch series are fixes for issues I found during recent tests of
an MCP2221 board.

- you can confuse the kernel driver when using the chip from user mode
  via /dev/hidrawX, typically leading to a NULL pointer dereference in
  the driver's HID raw event handler

- the driver needs > 15s to initialize because the HID raw handling is
  not enabled during initialization of the GPIO part

- the rxbuf pointer can become invalid or even stale if the device would
  send unsolicited reports

Changes in v2:

- removed: data shared with the bottom half code is not protected from
  concurrent access

Feedback if this is actually needed or not would be appreciated.

- rebased on linux-hid/for-6.2/mcp2221

Enrik Berkhan (3):
  HID: mcp2221: don't connect hidraw
  HID: mcp2221: enable HID I/O during GPIO probe
  HID: mcp2221: avoid stale rxbuf pointer

 drivers/hid/hid-mcp2221.c | 51 +++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 10 deletions(-)

base-commit: 3d74c9eca1a2bda03e45f18d13154ac3e0dfba85
-- 
2.34.1

[PATCH v2 1/3] HID: mcp2221: don't connect hidraw

[Enrik Berkhan]

The MCP2221 driver should not connect to the hidraw userspace interface,
as it needs exclusive access to the chip.

If you want to use /dev/hidrawX with the MCP2221, you need to avoid
binding this driver to the device and use the hid generic driver instead
(e.g. using udev rules).

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 5886543b17f3..e61dd039354b 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1110,12 +1110,19 @@ static int mcp2221_probe(struct hid_device *hdev,
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	/*
+	 * This driver uses the .raw_event callback and therefore does not need any
+	 * HID_CONNECT_xxx flags.
+	 */
+	ret = hid_hw_start(hdev, 0);
 	if (ret) {
 		hid_err(hdev, "can't start hardware\n");
 		return ret;
 	}
 
+	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+			hdev->version & 0xff, hdev->name, hdev->phys);
+
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "can't open device\n");
@@ -1145,8 +1152,7 @@ static int mcp2221_probe(struct hid_device *hdev,
 	mcp->adapter.retries = 1;
 	mcp->adapter.dev.parent = &hdev->dev;
 	snprintf(mcp->adapter.name, sizeof(mcp->adapter.name),
-			"MCP2221 usb-i2c bridge on hidraw%d",
-			((struct hidraw *)hdev->hidraw)->minor);
+			"MCP2221 usb-i2c bridge");
 
 	ret = devm_i2c_add_adapter(&hdev->dev, &mcp->adapter);
 	if (ret) {
-- 
2.34.1

[PATCH v2 2/3] HID: mcp2221: enable HID I/O during GPIO probe

[Enrik Berkhan]

As soon as the GPIO driver part will be enabled in mcp2221_probe(), the
first HID reports will be exchanged with the chip because the GPIO
driver immediately calls mcp_gpio_get_direction(). HID I/O has to be
enabled explicitly during mcp2221_probe() to receive response reports.

Otherwise, all four mcp_gpio_get_direction() calls will run into the
four second timeout of mcp_send_report(), which will block the driver
for about 16s during startup.

A very similar patch appeared some time ago in
https://lore.kernel.org/r/20210818152743.163929-1-tobias.junghans@inhub.de
which obviously got lost somehow.

CC: Tobias Junghans <tobias.junghans@inhub.de>

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e61dd039354b..0705526231ec 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1178,6 +1178,9 @@ static int mcp2221_probe(struct hid_device *hdev,
 	mcp->gc->can_sleep = 1;
 	mcp->gc->parent = &hdev->dev;
 
+	/* Enable reception of HID reports during GPIO initialization */
+	hid_device_io_start(hdev);
+
 	ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH v2 3/3] HID: mcp2221: avoid stale rxbuf pointer

[Enrik Berkhan]

In case the MCP2221 driver receives an unexpected read complete report
from the device, the data should not be copied to mcp->rxbuf. The
pointer might be NULL or even stale, having been set during an earlier
transaction.

Further, some bounds checking has been added.

Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
---
 drivers/hid/hid-mcp2221.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 0705526231ec..471b29c3c501 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -98,6 +98,7 @@ struct mcp2221 {
 	u8 *rxbuf;
 	u8 txbuf[64];
 	int rxbuf_idx;
+	int rxbuf_len;
 	int status;
 	u8 cur_i2c_clk_div;
 	struct gpio_chip *gc;
@@ -294,11 +295,12 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
 		mcp->rxbuf = smbus_buf;
 	}
 
+	mcp->rxbuf_len = total_len;
+	mcp->rxbuf_idx = 0;
+
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);
 	if (ret)
-		return ret;
-
-	mcp->rxbuf_idx = 0;
+		goto out_invalidate_rxbuf;
 
 	do {
 		memset(mcp->txbuf, 0, 4);
@@ -306,15 +308,20 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
 
 		ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
 		if (ret)
-			return ret;
+			goto out_invalidate_rxbuf;
 
 		ret = mcp_chk_last_cmd_status(mcp);
 		if (ret)
-			return ret;
+			goto out_invalidate_rxbuf;
 
 		usleep_range(980, 1000);
 	} while (mcp->rxbuf_idx < total_len);
 
+out_invalidate_rxbuf:
+	mcp->rxbuf = NULL;
+	mcp->rxbuf_len = 0;
+	mcp->rxbuf_idx = 0;
+
 	return ret;
 }
 
@@ -499,8 +506,12 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 
 			mcp->rxbuf_idx = 0;
 			mcp->rxbuf = data->block;
+			mcp->rxbuf_len = sizeof(data->block);
 			mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
 			ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
+			mcp->rxbuf_idx = 0;
+			mcp->rxbuf = NULL;
+			mcp->rxbuf_len = 0;
 			if (ret)
 				goto exit;
 		} else {
@@ -522,8 +533,12 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 
 			mcp->rxbuf_idx = 0;
 			mcp->rxbuf = data->block;
+			mcp->rxbuf_len = sizeof(data->block);
 			mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
 			ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
+			mcp->rxbuf_idx = 0;
+			mcp->rxbuf = NULL;
+			mcp->rxbuf_len = 0;
 			if (ret)
 				goto exit;
 		} else {
@@ -734,6 +749,7 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 				struct hid_report *report, u8 *data, int size)
 {
 	u8 *buf;
+	int len;
 	struct mcp2221 *mcp = hid_get_drvdata(hdev);
 
 	switch (data[0]) {
@@ -792,9 +808,15 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 				break;
 			}
 			if (data[2] == MCP2221_I2C_READ_COMPL) {
+				if (mcp->rxbuf == NULL || mcp->rxbuf_idx >= mcp->rxbuf_len)
+					return 1; /* no complete() in this case */
+
 				buf = mcp->rxbuf;
-				memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
-				mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
+				len = data[3];
+				if (len > mcp->rxbuf_len - mcp->rxbuf_idx)
+					len = mcp->rxbuf_len - mcp->rxbuf_idx;
+				memcpy(&buf[mcp->rxbuf_idx], &data[4], len);
+				mcp->rxbuf_idx = mcp->rxbuf_idx + len;
 				mcp->status = 0;
 				break;
 			}
-- 
2.34.1

Re: [PATCH v2 1/3] HID: mcp2221: don't connect hidraw

[Benjamin Tissoires]

On Nov 03 2022, Enrik Berkhan wrote:
> The MCP2221 driver should not connect to the hidraw userspace interface,
> as it needs exclusive access to the chip.
> 
> If you want to use /dev/hidrawX with the MCP2221, you need to avoid
> binding this driver to the device and use the hid generic driver instead
> (e.g. using udev rules).
> 
> Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
> ---

Given the NULL pointer deference report at
https://lore.kernel.org/all/79152feb-bcbc-9e3e-e776-13170ae4ef40@vigem.de/

I have added:
Reported-by: Sven Zühlsdorf <sven.zuehlsdorf@vigem.de>
And applied this one only in the series in for-6.2/upstream-fixes.

Before applying the rest I'd rather have some external reviews of this
series.

Cheers,
Benjamin

>  drivers/hid/hid-mcp2221.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index 5886543b17f3..e61dd039354b 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -1110,12 +1110,19 @@ static int mcp2221_probe(struct hid_device *hdev,
>  		return ret;
>  	}
>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	/*
> +	 * This driver uses the .raw_event callback and therefore does not need any
> +	 * HID_CONNECT_xxx flags.
> +	 */
> +	ret = hid_hw_start(hdev, 0);
>  	if (ret) {
>  		hid_err(hdev, "can't start hardware\n");
>  		return ret;
>  	}
>  
> +	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
> +			hdev->version & 0xff, hdev->name, hdev->phys);
> +
>  	ret = hid_hw_open(hdev);
>  	if (ret) {
>  		hid_err(hdev, "can't open device\n");
> @@ -1145,8 +1152,7 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	mcp->adapter.retries = 1;
>  	mcp->adapter.dev.parent = &hdev->dev;
>  	snprintf(mcp->adapter.name, sizeof(mcp->adapter.name),
> -			"MCP2221 usb-i2c bridge on hidraw%d",
> -			((struct hidraw *)hdev->hidraw)->minor);
> +			"MCP2221 usb-i2c bridge");
>  
>  	ret = devm_i2c_add_adapter(&hdev->dev, &mcp->adapter);
>  	if (ret) {
> -- 
> 2.34.1
>