Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] HID: hid-eweadn: returning -ENODEV in eweadn_probe() unbinds sibling interfaces
- [High] HID: hid-eweadn: unchecked to_usb_interface() cast in eweadn_probe()
- [Low] HID: hid-eweadn: hardcoded USB IDs instead of hid-ids.h macros in eweadn_devices
--
commit d81d5f8f2cf2bc9851adb951ad34ae0654a99eee
Author: Liang Haowen [off-list ref]
HID: add driver for EWEADN G5 mice
This patch adds a HID driver for EWEADN G5 mice (0xa8a5:2255 and
0xa8a4:2255) that replaces the broken report descriptor on USB interface 0
with a standard mouse report descriptor so hid-core can parse motion and
button events.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-eweadn.c b/drivers/hid/hid-eweadn.c
new file mode 100644
index 0000000000000..6aacfc5dbe8ac
--- /dev/null
+++ b/drivers/hid/hid-eweadn.c
[ ... ]
quoted hunk ↗ jump to hunk
+static const __u8 *eweadn_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+ unsigned int *rsize)
+{
+ hid_info(hdev, "fixing up EWEADN G5 mouse report descriptor\n");
+ *rsize = sizeof(eweadn_rdesc);
+ return eweadn_rdesc;
+}
+
+static int eweadn_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct usb_interface *intf;
+ int ret;
+
+ intf = to_usb_interface(hdev->dev.parent);
[Severity: High]
Can to_usb_interface(hdev->dev.parent) be called here without first
checking hid_is_usb(hdev)?
Although eweadn_devices uses HID_USB_DEVICE(), that only matches
hdev->bus == BUS_USB, hdev->vendor, and hdev->product (or a device can be
bound via sysfs). A virtual HID device created via /dev/uhid in
uhid_dev_create2() (drivers/hid/uhid.c) can specify bus = BUS_USB and
matching vendor/product IDs while setting hdev->dev.parent to
uhid_misc.this_device rather than a struct usb_interface.
Without a hid_is_usb(hdev) check, container_of_const() on hdev->dev.parent
and dereferencing intf->cur_altsetting->desc.bInterfaceNumber will read out
of bounds and dereference an invalid pointer.
+ if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
+ return -ENODEV; /* keyboard and config stay on hid-generic */
[Severity: High]
Does returning -ENODEV in eweadn_probe() actually allow interfaces 1 and 2
to stay on hid-generic, or does it leave them completely unbound?
Each USB interface of the composite device creates a separate struct
hid_device with identical bus, vendor, and product IDs, so
hid_match_device(hdev, &eweadn_driver) matches eweadn_devices for all three
interfaces.
In drivers/hid/hid-generic.c, hid_generic_match() checks whether any other
registered HID driver's id_table matches the device:
drivers/hid/hid-generic.c:hid_generic_match() {
...
if (bus_for_each_drv(&hid_bus_type, NULL, hdev, __check_hid_generic))
return false;
return true;
}
where __check_hid_generic() returns 1 whenever hid_match_device(hdev, hdrv)
is non-NULL, regardless of whether hdrv->probe() later returns -ENODEV. In
addition, when eweadn_driver is registered, __hid_register_driver() calls
__hid_bus_reprobe_drivers(), which sees !hid_generic_match() and calls
device_reprobe(), detaching hid-generic from interfaces 1 and 2. Because
eweadn_probe() then returns -ENODEV for bInterfaceNumber != 0, interfaces 1
and 2 are left with no bound driver at all.
Instead of rejecting non-zero interfaces in a custom .probe callback,
should the bInterfaceNumber == 0 check (guarded by hid_is_usb(hdev)) be
done inside eweadn_report_fixup() so non-zero interfaces keep their
original report descriptor while binding normally?
quoted hunk ↗ jump to hunk
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "parse failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hdev, "hw start failed: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct hid_device_id eweadn_devices[] = {
+ { HID_USB_DEVICE(0xa8a5, 0x2255) }, /* original receiver */
+ { HID_USB_DEVICE(0xa8a4, 0x2255) }, /* replacement unit, wired mouse */
+ { }
+};
[Severity: Low]
This isn't a bug, but drivers/hid/hid-ids.h already defines:
#define USB_VENDOR_ID_YJXCHIP 0xa8a5
#define USB_DEVICE_ID_YJXCHIP_MOUSE 0x2255
Should hid-eweadn.c include "hid-ids.h" and use those macros (adding a
corresponding macro for 0xa8a4) instead of raw hex constants, and mark
eweadn_rdesc[] as const?
--
Sashiko AI review · https://sashiko.dev/#/patchset/6ae7529d.10abdf86.7545b.3e9a@mx.google.com?part=1