[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

[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