| Message ID | 20210225145215.3438202-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers/hid: fix for the big hid report length | expand |
On Thu, Feb 25, 2021 at 08:52:15PM +0600, Sabyrzhan Tasbolatov wrote: > syzbot found WARNING in hid_alloc_report_buf[1] when the raw buffer for > report is kmalloc() allocated with length > KMALLOC_MAX_SIZE, causing > order >= MAX_ORDER condition: > > u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags) > { > /* > * 7 extra bytes are necessary to achieve proper functionality > * of implement() working on 8 byte chunks > */ > > u32 len = hid_report_len(report) + 7; > > return kmalloc(len, flags); > > The restriction with HID_MAX_BUFFER_SIZE (16kb) is, seems, a valid max > limit. I've come up with this in all hid_report_len() xrefs. > > The fix inside hid_report_len(), not in *hid_alloc_report_buf() is also > fixing out-of-bounds here in memcpy(): > > statc int hid_submit_ctrl(..) > { > .. > len = hid_report_len(report); > if (dir == USB_DIR_OUT) { > .. > if (raw_report) { > memcpy(usbhid->ctrlbuf, raw_report, len); > .. > > So I've decided to return HID_MAX_BUFFER_SIZE if it the report length is > bigger than limit, otherwise the return the report length. > > [1] > Call Trace: > alloc_pages include/linux/gfp.h:547 [inline] > kmalloc_order+0x40/0x130 mm/slab_common.c:837 > kmalloc_order_trace+0x15/0x70 mm/slab_common.c:853 > kmalloc_large include/linux/slab.h:481 [inline] > __kmalloc+0x257/0x330 mm/slub.c:3974 > kmalloc include/linux/slab.h:557 [inline] > hid_alloc_report_buf+0x70/0xa0 drivers/hid/hid-core.c:1648 > __usbhid_submit_report drivers/hid/usbhid/hid-core.c:590 [inline] > > Reported-by: syzbot+ab02336a647181a886a6@syzkaller.appspotmail.com > Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com> > --- > drivers/hid/usbhid/hid-core.c | 2 +- > include/linux/hid.h | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c > index 86257ce6d619..4e9077363c96 100644 > --- a/drivers/hid/usbhid/hid-core.c > +++ b/drivers/hid/usbhid/hid-core.c > @@ -374,7 +374,7 @@ static int hid_submit_ctrl(struct hid_device *hid) > raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report; > dir = usbhid->ctrl[usbhid->ctrltail].dir; > > - len = ((report->size - 1) >> 3) + 1 + (report->id > 0); > + len = hid_report_len(report); > if (dir == USB_DIR_OUT) { > usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0); > usbhid->urbctrl->transfer_buffer_length = len; > diff --git a/include/linux/hid.h b/include/linux/hid.h > index c39d71eb1fd0..509a6ffdca00 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -1156,7 +1156,9 @@ static inline void hid_hw_wait(struct hid_device *hdev) > static inline u32 hid_report_len(struct hid_report *report) > { > /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */ > - return ((report->size - 1) >> 3) + 1 + (report->id > 0); > + u32 len = ((report->size - 1) >> 3) + 1 + (report->id > 0); > + > + return len > HID_MAX_BUFFER_SIZE ? HID_MAX_BUFFER_SIZE : len; Won't this cause silent errors? How about instead just rejecting any device which includes a report whose length is too big (along with an line in the system log explaining what's wrong)? Alan Stern
On Thu, 25 Feb 2021 10:59:14 -0500, Alan Stern wrote: > Won't this cause silent errors? Agree. But there are already such as cases like in: // net/bluetooth/hidp/core.c static void hidp_process_report(..) { .. if (len > HID_MAX_BUFFER_SIZE) len = HID_MAX_BUFFER_SIZE; .. // drivers/hid/hid-core.c int hid_report_raw_event(..) { .. rsize = hid_compute_report_size(report); if (report_enum->numbered && rsize >= HID_MAX_BUFFER_SIZE) rsize = HID_MAX_BUFFER_SIZE - 1; else if (rsize > HID_MAX_BUFFER_SIZE) rsize = HID_MAX_BUFFER_SIZE; .. // drivers/staging/greybus/hid.c static int gb_hid_start(..) { .. if (bufsize > HID_MAX_BUFFER_SIZE) bufsize = HID_MAX_BUFFER_SIZE; .. > How about instead just rejecting any device which includes a report > whose length is too big (along with an line in the system log explaining > what's wrong)? Not everywhere, but there are already such as logs when > HID_MAX_BUFFER_SIZE // drivers/hid/hidraw.c static ssize_t hidraw_send_report(..) { .. if (count > HID_MAX_BUFFER_SIZE) { hid_warn(dev, "pid %d passed too large report\n", task_pid_nr(current)); ret = -EINVAL; goto out; } I've just noticed that hid_compute_report_size() doing the same thing as hid_report_len(). So I will replace it with latter one with length check. So in [PATCH v2] I will do following: 1. replace hid_compute_report_size() with hid_report_len() 2. in hid_report_len() we can hid_warn() if length > HID_MAX_BUFFER_SIZE, and return HID_MAX_BUFFER_SIZE. Or we can return 0 in hid_report_len() to let functions like hid_hw_raw_request(), hid_hw_output_report() to validate invalid report length and return -EINVAL. Though I'll need to add !length missing checks in other places. Please let me know what it's preferred way in 2nd step.
On Fri, Feb 26, 2021 at 02:13:36PM +0600, Sabyrzhan Tasbolatov wrote: > On Thu, 25 Feb 2021 10:59:14 -0500, Alan Stern wrote: > > Won't this cause silent errors? > > Agree. But there are already such as cases like in: > > // net/bluetooth/hidp/core.c > static void hidp_process_report(..) > { > .. > if (len > HID_MAX_BUFFER_SIZE) > len = HID_MAX_BUFFER_SIZE; > .. > > // drivers/hid/hid-core.c > int hid_report_raw_event(..) > { > .. > rsize = hid_compute_report_size(report); > > if (report_enum->numbered && rsize >= HID_MAX_BUFFER_SIZE) > rsize = HID_MAX_BUFFER_SIZE - 1; > else if (rsize > HID_MAX_BUFFER_SIZE) > rsize = HID_MAX_BUFFER_SIZE; > .. > > // drivers/staging/greybus/hid.c > static int gb_hid_start(..) > { > .. > if (bufsize > HID_MAX_BUFFER_SIZE) > bufsize = HID_MAX_BUFFER_SIZE; > .. > > > How about instead just rejecting any device which includes a report > > whose length is too big (along with an line in the system log explaining > > what's wrong)? > > Not everywhere, but there are already such as logs when > HID_MAX_BUFFER_SIZE > > // drivers/hid/hidraw.c > static ssize_t hidraw_send_report(..) > { > .. > if (count > HID_MAX_BUFFER_SIZE) { > hid_warn(dev, "pid %d passed too large report\n", > task_pid_nr(current)); > ret = -EINVAL; > goto out; > } > > > I've just noticed that hid_compute_report_size() doing the same thing as > hid_report_len(). So I will replace it with latter one with length check. > > So in [PATCH v2] I will do following: > > 1. replace hid_compute_report_size() with hid_report_len() > > 2. in hid_report_len() we can hid_warn() if length > HID_MAX_BUFFER_SIZE, > and return HID_MAX_BUFFER_SIZE. Or we can return 0 in hid_report_len() to let > functions like hid_hw_raw_request(), hid_hw_output_report() to validate > invalid report length and return -EINVAL. Though I'll need to add !length > missing checks in other places. > > Please let me know what it's preferred way in 2nd step. It's been too long since I worked on this stuff; you should check with the maintainers. Another thing to consider: There probably are devices with multiple reports, where one of the reports is too big but people only want to use the other, smaller reports. For situations like that, we don't want to reject the entire device. I don't know if parsing a partiall part is a good thing to do. Alan Stern
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 86257ce6d619..4e9077363c96 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -374,7 +374,7 @@ static int hid_submit_ctrl(struct hid_device *hid) raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report; dir = usbhid->ctrl[usbhid->ctrltail].dir; - len = ((report->size - 1) >> 3) + 1 + (report->id > 0); + len = hid_report_len(report); if (dir == USB_DIR_OUT) { usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0); usbhid->urbctrl->transfer_buffer_length = len; diff --git a/include/linux/hid.h b/include/linux/hid.h index c39d71eb1fd0..509a6ffdca00 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1156,7 +1156,9 @@ static inline void hid_hw_wait(struct hid_device *hdev) static inline u32 hid_report_len(struct hid_report *report) { /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */ - return ((report->size - 1) >> 3) + 1 + (report->id > 0); + u32 len = ((report->size - 1) >> 3) + 1 + (report->id > 0); + + return len > HID_MAX_BUFFER_SIZE ? HID_MAX_BUFFER_SIZE : len; } int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
syzbot found WARNING in hid_alloc_report_buf[1] when the raw buffer for report is kmalloc() allocated with length > KMALLOC_MAX_SIZE, causing order >= MAX_ORDER condition: u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags) { /* * 7 extra bytes are necessary to achieve proper functionality * of implement() working on 8 byte chunks */ u32 len = hid_report_len(report) + 7; return kmalloc(len, flags); The restriction with HID_MAX_BUFFER_SIZE (16kb) is, seems, a valid max limit. I've come up with this in all hid_report_len() xrefs. The fix inside hid_report_len(), not in *hid_alloc_report_buf() is also fixing out-of-bounds here in memcpy(): statc int hid_submit_ctrl(..) { .. len = hid_report_len(report); if (dir == USB_DIR_OUT) { .. if (raw_report) { memcpy(usbhid->ctrlbuf, raw_report, len); .. So I've decided to return HID_MAX_BUFFER_SIZE if it the report length is bigger than limit, otherwise the return the report length. [1] Call Trace: alloc_pages include/linux/gfp.h:547 [inline] kmalloc_order+0x40/0x130 mm/slab_common.c:837 kmalloc_order_trace+0x15/0x70 mm/slab_common.c:853 kmalloc_large include/linux/slab.h:481 [inline] __kmalloc+0x257/0x330 mm/slub.c:3974 kmalloc include/linux/slab.h:557 [inline] hid_alloc_report_buf+0x70/0xa0 drivers/hid/hid-core.c:1648 __usbhid_submit_report drivers/hid/usbhid/hid-core.c:590 [inline] Reported-by: syzbot+ab02336a647181a886a6@syzkaller.appspotmail.com Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com> --- drivers/hid/usbhid/hid-core.c | 2 +- include/linux/hid.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)