[PATCH] HID: usbhid: add HID_QUIRK_SKIP_FEATURE_REPORT_ID

[PATCH] HID: usbhid: add HID_QUIRK_SKIP_FEATURE_REPORT_ID

[Fedor Gubenkov]

Some HID dongles implement a stateful challenge/response over HID
feature reports: a SET_REPORT(Feature) carrying the challenge,
immediately followed by a GET_REPORT(Feature) returning the response.

One such dongle (3689:8762, reporting itself as "USBKey"; used by
several automotive key-programming tools) rejects SET_REPORT(Feature)
when the report ID byte is present in the payload. For a numbered
feature report usbhid puts the report ID both into the control-transfer
wValue and as the first byte of the data, i.e. it sends 21 bytes for a
20-byte report. The device then STALLs (EPIPE) the following
GET_REPORT(Feature), which breaks the challenge/response and makes the
dongle unusable - also for Wine applications relying on it.

A raw transfer that keeps the report ID only in wValue and omits it
from the SET_REPORT(Feature) payload (20 bytes) works: the device
accepts the challenge and returns the response. Confirmed with usbmon:

  usbhid: Co .. 0302 0000 0015 21 = 02 11 ..  -> GET: C -32 (STALL)
  libusb: Co .. 0302 0000 0014 20 = 10 11 ..  -> GET: C 0   (response)

Mirror the existing HID_QUIRK_SKIP_OUTPUT_REPORT_ID handling for feature
reports via a new HID_QUIRK_SKIP_FEATURE_REPORT_ID quirk and apply it to
the 3689:8762 dongle.

Signed-off-by: Fedor Gubenkov <writesfaster@gmail.com>
---
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -408,6 +408,7 @@
 #define HID_QUIRK_X_INVERT			BIT(12)
 #define HID_QUIRK_Y_INVERT			BIT(13)
 #define HID_QUIRK_IGNORE_MOUSE			BIT(14)
+#define HID_QUIRK_SKIP_FEATURE_REPORT_ID	BIT(15)
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		BIT(16)
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		BIT(17)
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	BIT(18)
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -914,8 +914,10 @@
 	int ret, skipped_report_id = 0;
 
 	/* Byte 0 is the report number. Report data starts at byte 1.*/
-	if ((rtype == HID_OUTPUT_REPORT) &&
-	    (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
+	if (((rtype == HID_OUTPUT_REPORT) &&
+	     (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID)) ||
+	    ((rtype == HID_FEATURE_REPORT) &&
+	     (hid->quirks & HID_QUIRK_SKIP_FEATURE_REPORT_ID)))
 		buf[0] = 0;
 	else
 		buf[0] = reportnum;
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1623,6 +1623,9 @@
 #define USB_DEVICE_ID_RAPHNET_2NES2SNES	0x0002
 #define USB_DEVICE_ID_RAPHNET_4NES4SNES	0x0003
 
+#define USB_VENDOR_ID_USBKEY		0x3689
+#define USB_DEVICE_ID_USBKEY_DONGLE	0x8762
+
 #define USB_VENDOR_ID_UGTIZER			0x2179
 #define USB_DEVICE_ID_UGTIZER_TABLET_GP0610	0x0053
 #define USB_DEVICE_ID_UGTIZER_TABLET_GT5040	0x0077
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -211,6 +211,7 @@
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_KNA5), HID_QUIRK_MULTI_INPUT },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_TWA60), HID_QUIRK_MULTI_INPUT },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UGTIZER, USB_DEVICE_ID_UGTIZER_TABLET_WP5540), HID_QUIRK_MULTI_INPUT },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_USBKEY, USB_DEVICE_ID_USBKEY_DONGLE), HID_QUIRK_SKIP_FEATURE_REPORT_ID },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_VRS, USB_DEVICE_ID_VRS_R295), HID_QUIRK_ALWAYS_POLL },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_10_6_INCH), HID_QUIRK_MULTI_INPUT },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH), HID_QUIRK_MULTI_INPUT },

Re: [PATCH] HID: usbhid: add HID_QUIRK_SKIP_FEATURE_REPORT_ID

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: missing HID_QUIRK_SKIP_FEATURE_REPORT_ID check in usbhid_get_raw_report
--

commit b5d7d6ff27fe5a08bcc6f67c8ed596cb786d6404
Author: Fedor Gubenkov <writesfaster@gmail.com>

HID: usbhid: add HID_QUIRK_SKIP_FEATURE_REPORT_ID

Some HID dongles implement a stateful challenge/response over HID
feature reports. This patch introduces a quirk to omit the report ID from
the SET_REPORT payload, fixing a stall when interacting with the USBKey dongle.

> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -914,8 +914,10 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
>  	int ret, skipped_report_id = 0;
>  
>  	/* Byte 0 is the report number. Report data starts at byte 1.*/
> -	if ((rtype == HID_OUTPUT_REPORT) &&
> -	    (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
> +	if (((rtype == HID_OUTPUT_REPORT) &&
> +	     (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID)) ||
> +	    ((rtype == HID_FEATURE_REPORT) &&
> +	     (hid->quirks & HID_QUIRK_SKIP_FEATURE_REPORT_ID)))
>  		buf[0] = 0;
>  	else
>  		buf[0] = reportnum;

[Severity: Medium]
Since this quirk is used for a challenge/response over feature reports,
does usbhid_get_raw_report() also need to be updated to handle the response?

If the device omits the report ID in its response, it appears
usbhid_get_raw_report() will not offset the buffer for numbered reports:

drivers/hid/usbhid/hid-core.c:usbhid_get_raw_report() {
	/* Byte 0 is the report number. Report data starts at byte 1.*/
	buf[0] = report_number;
	if (report_number == 0x0) {
		/* Offset the return buffer by 1, so that the report ID
		   will remain in byte 0. */
		buf++;
		count--;
		skipped_report_id = 1;
	}
...
}

Will this cause usb_control_msg() to overwrite buf[0] (the report ID) with the
first byte of the device's raw payload, resulting in a misaligned payload for
user-space applications reading these feature reports?

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

Re: [PATCH] HID: usbhid: add HID_QUIRK_SKIP_FEATURE_REPORT_ID

[Fedor Gubenkov]

Please drop this patch; I'm NAK-ing my own submission.

After further testing against the actual device, the premise of this
patch is wrong. The dongle (3689:8762) *requires* the report ID byte in
the SET_REPORT(Feature) payload. Captured at the USB level:

  - SET_REPORT(Feature) with the report ID present (21 bytes): the device
    returns a valid, challenge-dependent response.
  - SET_REPORT(Feature) with the report ID stripped (20 bytes) - exactly
    what HID_QUIRK_SKIP_FEATURE_REPORT_ID produces - the device returns a
    single constant byte (0xe7) for every challenge, i.e. an invalid
    reply.

So the unmodified usbhid behaviour (report ID included, as mandated by
the HID spec for numbered reports) is already correct for this device,
and the quirk breaks the only device it targets. My earlier "the 20-byte
transfer works" observation was a flawed comparison that used a different
challenge and never actually elicited a valid response.

Sorry for the noise.

Thanks,
Fedor