drivers/hid/hid-mcp2221.c | 114 ++++++++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 17 deletions(-)
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
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
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.1On 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
>
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.1In 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© 2016 - 2025 Red Hat, Inc.