| Message ID | 20250307045449.745634-1-linuxhid@cosmicgizmosystems.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] HID: usbhid: Eliminate recurrent out-of-bounds bug in usbhid_parse() | expand |
Hi Terry, kernel test robot noticed the following build warnings: [auto build test WARNING on 58c9bf3363e596d744f56616d407278ef5f97f5a] url: https://github.com/intel-lab-lkp/linux/commits/Terry-Junge/HID-usbhid-Eliminate-recurrent-out-of-bounds-bug-in-usbhid_parse/20250307-130514 base: 58c9bf3363e596d744f56616d407278ef5f97f5a patch link: https://lore.kernel.org/r/20250307045449.745634-1-linuxhid%40cosmicgizmosystems.com patch subject: [PATCH v1] HID: usbhid: Eliminate recurrent out-of-bounds bug in usbhid_parse() config: s390-randconfig-r133-20250308 (https://download.01.org/0day-ci/archive/20250309/202503090701.715nV1DW-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce: (https://download.01.org/0day-ci/archive/20250309/202503090701.715nV1DW-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503090701.715nV1DW-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hid/usbhid/hid-core.c:1055:4: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] hdesc->bNumDescriptors - 1); ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/hid.h:1239:31: note: expanded from macro 'hid_warn' dev_warn(&(hid)->dev, fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ include/linux/dev_printk.h:156:70: note: expanded from macro 'dev_warn' dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap' _p_func(dev, fmt, ##__VA_ARGS__); \ ~~~ ^~~~~~~~~~~ 1 warning generated. vim +1055 drivers/hid/usbhid/hid-core.c 979 980 static int usbhid_parse(struct hid_device *hid) 981 { 982 struct usb_interface *intf = to_usb_interface(hid->dev.parent); 983 struct usb_host_interface *interface = intf->cur_altsetting; 984 struct usb_device *dev = interface_to_usbdev (intf); 985 struct hid_descriptor *hdesc; 986 struct hid_class_descriptor *hcdesc; 987 u32 quirks = 0; 988 unsigned int rsize = 0; 989 char *rdesc; 990 int ret; 991 992 quirks = hid_lookup_quirk(hid); 993 994 if (quirks & HID_QUIRK_IGNORE) 995 return -ENODEV; 996 997 /* Many keyboards and mice don't like to be polled for reports, 998 * so we will always set the HID_QUIRK_NOGET flag for them. */ 999 if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) { 1000 if (interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_KEYBOARD || 1001 interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) 1002 quirks |= HID_QUIRK_NOGET; 1003 } 1004 1005 if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) && 1006 (!interface->desc.bNumEndpoints || 1007 usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) { 1008 dbg_hid("class descriptor not present\n"); 1009 return -ENODEV; 1010 } 1011 1012 if (!hdesc->bNumDescriptors || 1013 hdesc->bLength != sizeof(*hdesc) + 1014 (hdesc->bNumDescriptors - 1) * sizeof(*hcdesc)) { 1015 dbg_hid("hid descriptor invalid, bLen=%hhu bNum=%hhu\n", 1016 hdesc->bLength, hdesc->bNumDescriptors); 1017 return -EINVAL; 1018 } 1019 1020 hid->version = le16_to_cpu(hdesc->bcdHID); 1021 hid->country = hdesc->bCountryCode; 1022 1023 if (hdesc->rpt_desc.bDescriptorType == HID_DT_REPORT) 1024 rsize = le16_to_cpu(hdesc->rpt_desc.wDescriptorLength); 1025 1026 if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) { 1027 dbg_hid("weird size of report descriptor (%u)\n", rsize); 1028 return -EINVAL; 1029 } 1030 1031 rdesc = kmalloc(rsize, GFP_KERNEL); 1032 if (!rdesc) 1033 return -ENOMEM; 1034 1035 hid_set_idle(dev, interface->desc.bInterfaceNumber, 0, 0); 1036 1037 ret = hid_get_class_descriptor(dev, interface->desc.bInterfaceNumber, 1038 HID_DT_REPORT, rdesc, rsize); 1039 if (ret < 0) { 1040 dbg_hid("reading report descriptor failed\n"); 1041 kfree(rdesc); 1042 goto err; 1043 } 1044 1045 ret = hid_parse_report(hid, rdesc, rsize); 1046 kfree(rdesc); 1047 if (ret) { 1048 dbg_hid("parsing report descriptor failed\n"); 1049 goto err; 1050 } 1051 1052 if (hdesc->bNumDescriptors > 1) 1053 hid_warn(intf, 1054 "%hhu unsupported optional hid class descriptors\n", > 1055 hdesc->bNumDescriptors - 1); 1056 1057 hid->quirks |= quirks; 1058 1059 return 0; 1060 err: 1061 return ret; 1062 } 1063
diff --git a/include/linux/hid.h b/include/linux/hid.h index cdc0dc13c87f..7abc8c74bdd5 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -738,8 +738,9 @@ struct hid_descriptor { __le16 bcdHID; __u8 bCountryCode; __u8 bNumDescriptors; + struct hid_class_descriptor rpt_desc; - struct hid_class_descriptor desc[1]; + struct hid_class_descriptor opt_descs[]; } __attribute__ ((packed)); #define HID_DEVICE(b, g, ven, prod) \ diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index 740311c4fa24..c7a05f842745 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -144,8 +144,8 @@ static struct hid_descriptor hidg_desc = { .bcdHID = cpu_to_le16(0x0101), .bCountryCode = 0x00, .bNumDescriptors = 0x1, - /*.desc[0].bDescriptorType = DYNAMIC */ - /*.desc[0].wDescriptorLenght = DYNAMIC */ + /*.rpt_desc.bDescriptorType = DYNAMIC */ + /*.rpt_desc.wDescriptorLength = DYNAMIC */ }; /* Super-Speed Support */ @@ -939,8 +939,8 @@ static int hidg_setup(struct usb_function *f, struct hid_descriptor hidg_desc_copy = hidg_desc; VDBG(cdev, "USB_REQ_GET_DESCRIPTOR: HID\n"); - hidg_desc_copy.desc[0].bDescriptorType = HID_DT_REPORT; - hidg_desc_copy.desc[0].wDescriptorLength = + hidg_desc_copy.rpt_desc.bDescriptorType = HID_DT_REPORT; + hidg_desc_copy.rpt_desc.wDescriptorLength = cpu_to_le16(hidg->report_desc_length); length = min_t(unsigned short, length, @@ -1210,8 +1210,8 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) * We can use hidg_desc struct here but we should not relay * that its content won't change after returning from this function. */ - hidg_desc.desc[0].bDescriptorType = HID_DT_REPORT; - hidg_desc.desc[0].wDescriptorLength = + hidg_desc.rpt_desc.bDescriptorType = HID_DT_REPORT; + hidg_desc.rpt_desc.wDescriptorLength = cpu_to_le16(hidg->report_desc_length); hidg_hs_in_ep_desc.bEndpointAddress = diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c index 0fb210e40a41..9eafff0b6ea4 100644 --- a/drivers/hid/hid-hyperv.c +++ b/drivers/hid/hid-hyperv.c @@ -192,7 +192,7 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device, goto cleanup; input_device->report_desc_size = le16_to_cpu( - desc->desc[0].wDescriptorLength); + desc->rpt_desc.wDescriptorLength); if (input_device->report_desc_size == 0) { input_device->dev_info_status = -EINVAL; goto cleanup; @@ -210,7 +210,7 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device, memcpy(input_device->report_desc, ((unsigned char *)desc) + desc->bLength, - le16_to_cpu(desc->desc[0].wDescriptorLength)); + le16_to_cpu(desc->rpt_desc.wDescriptorLength)); /* Send the ack */ memset(&ack, 0, sizeof(struct mousevsc_prt_msg)); diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index a6eb6fe6130d..e668143b559d 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -983,12 +983,11 @@ static int usbhid_parse(struct hid_device *hid) struct usb_host_interface *interface = intf->cur_altsetting; struct usb_device *dev = interface_to_usbdev (intf); struct hid_descriptor *hdesc; + struct hid_class_descriptor *hcdesc; u32 quirks = 0; unsigned int rsize = 0; char *rdesc; - int ret, n; - int num_descriptors; - size_t offset = offsetof(struct hid_descriptor, desc); + int ret; quirks = hid_lookup_quirk(hid); @@ -1010,20 +1009,19 @@ static int usbhid_parse(struct hid_device *hid) return -ENODEV; } - if (hdesc->bLength < sizeof(struct hid_descriptor)) { - dbg_hid("hid descriptor is too short\n"); + if (!hdesc->bNumDescriptors || + hdesc->bLength != sizeof(*hdesc) + + (hdesc->bNumDescriptors - 1) * sizeof(*hcdesc)) { + dbg_hid("hid descriptor invalid, bLen=%hhu bNum=%hhu\n", + hdesc->bLength, hdesc->bNumDescriptors); return -EINVAL; } hid->version = le16_to_cpu(hdesc->bcdHID); hid->country = hdesc->bCountryCode; - num_descriptors = min_t(int, hdesc->bNumDescriptors, - (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor)); - - for (n = 0; n < num_descriptors; n++) - if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT) - rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength); + if (hdesc->rpt_desc.bDescriptorType == HID_DT_REPORT) + rsize = le16_to_cpu(hdesc->rpt_desc.wDescriptorLength); if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) { dbg_hid("weird size of report descriptor (%u)\n", rsize); @@ -1051,6 +1049,11 @@ static int usbhid_parse(struct hid_device *hid) goto err; } + if (hdesc->bNumDescriptors > 1) + hid_warn(intf, + "%hhu unsupported optional hid class descriptors\n", + hdesc->bNumDescriptors - 1); + hid->quirks |= quirks; return 0;
Update struct hid_descriptor to better reflect the mandatory and optional parts of the HID Descriptor as per USB HID 1.11 specification. Note: the kernel currently does not parse any optional HID class descriptors, only the mandatory report descriptor. Update all references to member element desc[0] to rpt_desc. Add test to verify bLength and bNumDescriptors values are valid. Replace the for loop with direct access to the mandatory HID class descriptor member for the report descriptor. This eliminates the possibility of getting an out-of-bounds fault. Add a warning message if the HID descriptor contains any unsupported optional HID class descriptors. Reported-by: syzbot+c52569baf0c843f35495@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=c52569baf0c843f35495 Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug") Cc: stable@vger.kernel.org Signed-off-by: Terry Junge <linuxhid@cosmicgizmosystems.com> --- v1: Remove unnecessary for loop searching for the report descriptor size. base-commit: 58c9bf3363e596d744f56616d407278ef5f97f5a P.S. This is an alternative to the solution proposed by Nikita Zhandarovich <n.zhandarovich@fintech.ru> Link: https://lore.kernel.org/all/20250131151600.410242-1-n.zhandarovich@fintech.ru/ include/linux/hid.h | 3 ++- drivers/usb/gadget/function/f_hid.c | 12 ++++++------ drivers/hid/hid-hyperv.c | 4 ++-- drivers/hid/usbhid/hid-core.c | 25 ++++++++++++++----------- 4 files changed, 24 insertions(+), 20 deletions(-)