[PATCH v2 0/8] HID: validate report size in raw_event callbacks

[PATCH v2 0/8] HID: validate report size in raw_event callbacks

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

Several HID drivers access report data in their raw_event callbacks
without first verifying the buffer length. Since hid-core invokes each
driver's raw_event callback before it validates the report length in
hid_report_raw_event(), these drivers must perform their own bounds
checking to handle short reports safely.

This series adds report size validation to eight drivers whose
raw_event handlers read beyond the first byte before any length check.

Tristan Madani (8):
  HID: pxrc: validate report size in raw_event callback
  HID: alps: validate report size in raw_event callbacks
  HID: mcp2200: validate report size in raw_event callback
  HID: cougar: validate report size in raw_event callback
  HID: zydacron: validate report size in raw_event callback
  HID: oxp: validate report size in raw_event callbacks
  HID: cp2112: validate report size in raw_event callback
  HID: prodikeys: validate report size in raw_event callback

 drivers/hid/hid-alps.c      | 12 ++++++++++++
 drivers/hid/hid-cougar.c    |  3 +++
 drivers/hid/hid-cp2112.c    | 10 ++++++++++
 drivers/hid/hid-mcp2200.c   |  3 +++
 drivers/hid/hid-oxp.c       |  6 ++++++
 drivers/hid/hid-prodikeys.c |  4 ++++
 drivers/hid/hid-pxrc.c      |  3 +++
 drivers/hid/hid-zydacron.c  |  3 +++
 8 files changed, 44 insertions(+)

---
Changes v1 -> v2:
- Patch 2/8 (alps): Fixed U1_ABSOLUTE report bound from
  max_fingers * 5 + 5 to max_fingers * 5 + 3. The last byte accessed
  in the finger loop is contact[7] at data[max_fingers * 5 + 2],
  so the previous bound was two bytes too strict (reported by sashiko-bot).
- Patch 8/8 (prodikeys): Moved size checks into pcmidi_handle_report()
  with per-report-ID guards instead of a blanket size < 4 in
  pk_raw_event(). Report 0x03 (MIDI keyboard) passes size to
  pcmidi_handle_report3() which already handles variable-length
  reports; the blanket check dropped valid 3-byte single-note events
  (reported by sashiko-bot).

-- 
2.47.3

[PATCH v2 1/8] HID: pxrc: validate report size in raw_event callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

pxrc_raw_event() reads and writes data[1] and data[7] without
checking that the report buffer is large enough. A device sending a
short report could cause out-of-bounds memory access because
hid-core invokes raw_event before it validates the report length in
hid_report_raw_event().

Add a size check at the start of the function.

Fixes: acc3e34613da ("HID: Add driver for PhoenixRC Flight Controller")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-pxrc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-pxrc.c b/drivers/hid/hid-pxrc.c
index 71fe0c06ddcdf..e3755d8b85c22 100644
--- a/drivers/hid/hid-pxrc.c
+++ b/drivers/hid/hid-pxrc.c
@@ -55,6 +55,9 @@ static int pxrc_raw_event(struct hid_device *hdev, struct hid_report *report,
 {
 	struct pxrc_priv *priv = hid_get_drvdata(hdev);
 
+	if (size < 8)
+		return 0;
+
 	if (priv->alternate)
 		priv->slider = data[7];
 	else
-- 
2.47.3

[PATCH v2 2/8] HID: alps: validate report size in raw_event callbacks

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

t4_raw_event() and u1_raw_event() access report data at various
offsets and cast the buffer to struct t4_input_report without
verifying that the buffer is large enough. Since hid-core invokes
raw_event before validating the report length in
hid_report_raw_event(), each driver callback must check the size
itself.

Add size validation:
 - t4_raw_event: require sizeof(struct t4_input_report)
 - u1_raw_event: require at least 2 bytes before the switch
 - U1_ABSOLUTE_REPORT_ID: require max_fingers * 5 + 3 for the
   per-finger contact array (last byte accessed is contact[7] at
   data[max_fingers * 5 + 2])
 - U1_SP_ABSOLUTE_REPORT_ID: require 6 bytes for the coordinates

Fixes: 2562756dde55 ("HID: add Alps I2C HID Touchpad-Stick support")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-alps.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index 0556cb5645ebb..d984100093f6e 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -324,6 +324,10 @@ static int t4_raw_event(struct alps_dev *hdata, u8 *data, int size)
 
 	if (!data)
 		return 0;
+
+	if (size < sizeof(struct t4_input_report))
+		return 0;
+
 	for (i = 0; i < hdata->max_fingers; i++) {
 		x = p_report->contact[i].x_hi << 8 | p_report->contact[i].x_lo;
 		y = p_report->contact[i].y_hi << 8 | p_report->contact[i].y_lo;
@@ -363,6 +367,10 @@ static int u1_raw_event(struct alps_dev *hdata, u8 *data, int size)
 
 	if (!data)
 		return 0;
+
+	if (size < 2)
+		return 0;
+
 	switch (data[0]) {
 	case U1_MOUSE_REPORT_ID:
 		break;
@@ -370,6 +378,8 @@ static int u1_raw_event(struct alps_dev *hdata, u8 *data, int size)
 		break;
 	case U1_ABSOLUTE_REPORT_ID:
 	case U1_ABSOLUTE_REPORT_ID_SECD:
+		if (size < hdata->max_fingers * 5 + 3)
+			return 0;
 		for (i = 0; i < hdata->max_fingers; i++) {
 			u8 *contact = &data[i * 5];
 
@@ -407,6 +417,8 @@ static int u1_raw_event(struct alps_dev *hdata, u8 *data, int size)
 		return 1;
 
 	case U1_SP_ABSOLUTE_REPORT_ID:
+		if (size < 6)
+			return 0;
 		if (!hdata->input2)
 			return 0;
 		sp_x = get_unaligned_le16(data+2);
-- 
2.47.3

[PATCH v2 3/8] HID: mcp2200: validate report size in raw_event callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

mcp2200_raw_event() casts the report data to struct mcp_read_all_resp
and accesses fields up to io_port_val_bmap (offset 10) without
checking the buffer size. A device sending a short report could cause
out-of-bounds memory access because hid-core invokes raw_event before
it validates the report length in hid_report_raw_event().

Add a size check before the struct cast.

Fixes: 740329d7120f ("HID: mcp2200: added driver for GPIOs of MCP2200")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-mcp2200.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
index dafdd5b4a0797..78a08d4d8f0d0 100644
--- a/drivers/hid/hid-mcp2200.c
+++ b/drivers/hid/hid-mcp2200.c
@@ -299,6 +299,9 @@ static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct mcp2200 *mcp = hid_get_drvdata(hdev);
 	struct mcp_read_all_resp *all_resp;
 
+	if (size < sizeof(struct mcp_read_all_resp))
+		return 0;
+
 	switch (data[0]) {
 	case READ_ALL:
 		all_resp = (struct mcp_read_all_resp *) data;
-- 
2.47.3

[PATCH v2 4/8] HID: cougar: validate report size in raw_event callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

cougar_raw_event() accesses data[COUGAR_FIELD_CODE] (offset 1) and
data[COUGAR_FIELD_ACTION] (offset 2) without checking the report
size. A device sending a short report could cause out-of-bounds memory
access because hid-core invokes raw_event before it validates the
report length in hid_report_raw_event().

Add a size check at the start of the function.

Fixes: b8e759b8f6da ("HID: cougar: Add support for the Cougar 500k Gaming Keyboard")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-cougar.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
index ad027c45f1628..2dc135d0fd123 100644
--- a/drivers/hid/hid-cougar.c
+++ b/drivers/hid/hid-cougar.c
@@ -267,6 +267,9 @@ static int cougar_raw_event(struct hid_device *hdev, struct hid_report *report,
 	if (!cougar->special_intf || !shared)
 		return 0;
 
+	if (size < 3)
+		return 0;
+
 	if (!shared->enabled || !shared->input)
 		return -EPERM;
 
-- 
2.47.3

[PATCH v2 5/8] HID: zydacron: validate report size in raw_event callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

zc_raw_event() accesses data[1] without checking the report size.
A device sending a single-byte report could cause out-of-bounds memory
access because hid-core invokes raw_event before it validates the
report length in hid_report_raw_event().

Add a size check before accessing data[1].

Fixes: d0742abaa1c3 ("HID: add omitted hid-zydacron.c file")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-zydacron.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-zydacron.c b/drivers/hid/hid-zydacron.c
index 1aae80f848f50..203457398780c 100644
--- a/drivers/hid/hid-zydacron.c
+++ b/drivers/hid/hid-zydacron.c
@@ -116,6 +116,9 @@ static int zc_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	if (report->id == data[0] && (hdev->claimed & HID_CLAIMED_INPUT)) {
 
+		if (size < 2)
+			return 0;
+
 		/* break keys */
 		for (index = 0; index < 4; index++) {
 			key = zc->last_key[index];
-- 
2.47.3

[PATCH v2 6/8] HID: oxp: validate report size in raw_event callbacks

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

oxp_hid_raw_event_gen_1() and oxp_hid_raw_event_gen_2() cast the
report data to struct oxp_gen_1_rgb_report (11 bytes) and struct
oxp_gen_2_rgb_report (16 bytes) respectively, and access fields at
various offsets without checking the buffer size. A device sending a
short report could cause out-of-bounds memory access because hid-core
invokes raw_event before it validates the report length in
hid_report_raw_event().

Add size checks before the struct casts.

Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-oxp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 1e691ebc1199e..af9f60d45bf29 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -318,6 +318,9 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
 	struct led_classdev_mc *led_mc = drvdata.led_mc;
 	struct oxp_gen_1_rgb_report *rgb_rep;
 
+	if (size < sizeof(struct oxp_gen_1_rgb_report))
+		return 0;
+
 	if (data[1] != OXP_FID_GEN1_RGB_REPLY)
 		return 0;
 
@@ -391,6 +394,9 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
 	struct led_classdev_mc *led_mc = drvdata.led_mc;
 	struct oxp_gen_2_rgb_report *rgb_rep;
 
+	if (size < sizeof(struct oxp_gen_2_rgb_report))
+		return 0;
+
 	if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
 		return 0;
 
-- 
2.47.3

[PATCH v2 7/8] HID: cp2112: validate report size in raw_event callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

cp2112_raw_event() processes incoming HID reports without validating
the report size against the data it accesses. Since hid-core calls the
driver's raw_event callback before it validates the report size in
hid_report_raw_event(), the driver must perform its own bounds checking.

For CP2112_TRANSFER_STATUS_RESPONSE, the code casts data to struct
cp2112_xfer_status_report (7 bytes) and accesses fields at offsets 1-6.

For CP2112_DATA_READ_RESPONSE, the code reads data[1], data[2], and
then copies data[2] bytes (capped to 61) starting from data[3]. If the
report is shorter than 3 + read_length bytes, the memcpy reads beyond
the buffer boundary.

Add size validation for both report types:
 - Transfer status: require sizeof(struct cp2112_xfer_status_report)
 - Data read: require at least 3 bytes for the header, then cap the
   copy length to what is actually available in the buffer

Fixes: e932d817866770d4 ("HID: add hid-cp2112 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-cp2112.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 04379db93571b..2508c8ea8b693 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1430,6 +1430,10 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	switch (data[0]) {
 	case CP2112_TRANSFER_STATUS_RESPONSE:
+		if (size < sizeof(struct cp2112_xfer_status_report)) {
+			hid_err(hdev, "short transfer status report\n");
+			return 0;
+		}
 		hid_dbg(hdev, "xfer status: %02x %02x %04x %04x\n",
 			xfer->status0, xfer->status1,
 			be16_to_cpu(xfer->retries), be16_to_cpu(xfer->length));
@@ -1463,11 +1467,17 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
 		atomic_set(&dev->xfer_avail, 1);
 		break;
 	case CP2112_DATA_READ_RESPONSE:
+		if (size < 3) {
+			hid_err(hdev, "short data read response\n");
+			return 0;
+		}
 		hid_dbg(hdev, "read response: %02x %02x\n", data[1], data[2]);
 
 		dev->read_length = data[2];
 		if (dev->read_length > sizeof(dev->read_data))
 			dev->read_length = sizeof(dev->read_data);
+		if (dev->read_length > size - 3)
+			dev->read_length = size - 3;
 
 		memcpy(dev->read_data, &data[3], dev->read_length);
 		atomic_set(&dev->read_avail, 1);
-- 
2.47.3

[PATCH v2 8/8] HID: prodikeys: validate report size in raw_event callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

pcmidi_handle_report1() and pcmidi_handle_report4() read a 24-bit
bitmask from data[1..3] without verifying the report buffer is large
enough. Since hid-core invokes raw_event before validating the report
length in hid_report_raw_event(), the driver must check the buffer size
itself.

Add minimum size checks before calling pcmidi_handle_report1() and
pcmidi_handle_report4(). pcmidi_handle_report3() already handles
variable-length reports via its own size parameter and needs no
additional guard.

Fixes: 3a370ca1dcf8c ("HID: Prodikeys PC-MIDI HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-prodikeys.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index fba01e4fcab1c..925ca70f9cd4a 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -491,12 +491,16 @@ static int pcmidi_handle_report(
 
 	switch (report_id) {
 	case 0x01: /* midi keys (qwerty)*/
+		if (size < 4)
+			break;
 		ret = pcmidi_handle_report1(pm, data);
 		break;
 	case 0x03: /* midi keyboard (musical)*/
 		ret = pcmidi_handle_report3(pm, data, size);
 		break;
 	case 0x04: /* multimedia/midi keys (qwerty)*/
+		if (size < 4)
+			break;
 		ret = pcmidi_handle_report4(pm, data);
 		break;
 	}
-- 
2.47.3

Re: [PATCH v2 6/8] HID: oxp: validate report size in raw_event callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: oxp: out-of-bounds read in oxp_hid_raw_event() dev_dbg()
--

commit 5b821b0394212eadced6ac6427c3ff78bbef2d73
Author: Tristan Madani <tristan@talencesecurity.com>

HID: oxp: validate report size in raw_event callbacks

This commit adds report size checks to oxp_hid_raw_event_gen_1() and
oxp_hid_raw_event_gen_2() before casting the incoming report buffer to
struct oxp_gen_1_rgb_report and struct oxp_gen_2_rgb_report.

> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 1e691ebc1199e..af9f60d45bf29 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -318,6 +318,9 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
>  	struct led_classdev_mc *led_mc = drvdata.led_mc;
>  	struct oxp_gen_1_rgb_report *rgb_rep;
>  
> +	if (size < sizeof(struct oxp_gen_1_rgb_report))
> +		return 0;
> +
>  	if (data[1] != OXP_FID_GEN1_RGB_REPLY)
>  		return 0;
>  
> @@ -391,6 +394,9 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
>  	struct led_classdev_mc *led_mc = drvdata.led_mc;
>  	struct oxp_gen_2_rgb_report *rgb_rep;
>  
> +	if (size < sizeof(struct oxp_gen_2_rgb_report))
> +		return 0;
> +
>  	if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
>  		return 0;
>  

[Severity: Medium]
Can oxp_hid_raw_event() still read past the end of data before reaching
these checks when dynamic debug or DEBUG is enabled?

In drivers/hid/hid-oxp.c, the caller oxp_hid_raw_event() logs a fixed
OXP_PACKET_SIZE (64) bytes from data before dispatching to
oxp_hid_raw_event_gen_1() or oxp_hid_raw_event_gen_2():

drivers/hid/hid-oxp.c:oxp_hid_raw_event() {
	u16 up = get_usage_page(hdev);

	dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", OXP_PACKET_SIZE, data);

	switch (up) {
	case GEN1_USAGE_PAGE:
		return oxp_hid_raw_event_gen_1(hdev, report, data, size);
	case GEN2_USAGE_PAGE:
		return oxp_hid_raw_event_gen_2(hdev, report, data, size);
	...
}

When a short report arrives with size < OXP_PACKET_SIZE (such as a report
allocated for a smaller size via hid_alloc_report_buf() in __hid_request()
or injected via hid_bpf_input_report()), dev_dbg() will read 64 bytes out
of bounds from data before the new size checks in
oxp_hid_raw_event_gen_1() and oxp_hid_raw_event_gen_2() are reached.

Should dev_dbg() in oxp_hid_raw_event() pass size (or min(size,
OXP_PACKET_SIZE)) instead of OXP_PACKET_SIZE?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018220344.1987142-1-tristmd@gmail.com?part=6