HID: Fix slab-out-of-bounds read in hid_field_extract
diff mbox series

Message ID Pine.LNX.4.44L0.1912101622030.1647-100000@iolanthe.rowland.org
State Mainlined
Commit 8ec321e96e056de84022c032ffea253431a83c3c
Delegated to: Jiri Kosina
Headers show
Series
  • HID: Fix slab-out-of-bounds read in hid_field_extract
Related show

Commit Message

Alan Stern Dec. 10, 2019, 9:26 p.m. UTC
The syzbot fuzzer found a slab-out-of-bounds bug in the HID report
handler.  The bug was caused by a report descriptor which included a
field with size 12 bits and count 4899, for a total size of 7349
bytes.

The usbhid driver uses at most a single-page 4-KB buffer for reports.
In the test there wasn't any problem about overflowing the buffer,
since only one byte was received from the device.  Rather, the bug
occurred when the HID core tried to extract the data from the report
fields, which caused it to try reading data beyond the end of the
allocated buffer.

This patch fixes the problem by rejecting any report whose total
length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
for a possible report index).  In theory a device could have a report
longer than that, but if there was such a thing we wouldn't handle it 
correctly anyway.

Reported-and-tested-by: syzbot+09ef48aa58261464b621@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: <stable@vger.kernel.org>

---


[as1926]


 drivers/hid/hid-core.c |    6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jiri Kosina Dec. 11, 2019, 2:18 p.m. UTC | #1
On Tue, 10 Dec 2019, Alan Stern wrote:

> The syzbot fuzzer found a slab-out-of-bounds bug in the HID report
> handler.  The bug was caused by a report descriptor which included a
> field with size 12 bits and count 4899, for a total size of 7349
> bytes.
> 
> The usbhid driver uses at most a single-page 4-KB buffer for reports.
> In the test there wasn't any problem about overflowing the buffer,
> since only one byte was received from the device.  Rather, the bug
> occurred when the HID core tried to extract the data from the report
> fields, which caused it to try reading data beyond the end of the
> allocated buffer.
> 
> This patch fixes the problem by rejecting any report whose total
> length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
> for a possible report index).  In theory a device could have a report
> longer than that, but if there was such a thing we wouldn't handle it 
> correctly anyway.
> 
> Reported-and-tested-by: syzbot+09ef48aa58261464b621@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: <stable@vger.kernel.org>

Thanks for hunting this down Alan. Applied.
Alan Stern Dec. 11, 2019, 3:10 p.m. UTC | #2
On Wed, 11 Dec 2019, Jiri Kosina wrote:

> On Tue, 10 Dec 2019, Alan Stern wrote:
> 
> > The syzbot fuzzer found a slab-out-of-bounds bug in the HID report
> > handler.  The bug was caused by a report descriptor which included a
> > field with size 12 bits and count 4899, for a total size of 7349
> > bytes.
> > 
> > The usbhid driver uses at most a single-page 4-KB buffer for reports.
> > In the test there wasn't any problem about overflowing the buffer,
> > since only one byte was received from the device.  Rather, the bug
> > occurred when the HID core tried to extract the data from the report
> > fields, which caused it to try reading data beyond the end of the
> > allocated buffer.
> > 
> > This patch fixes the problem by rejecting any report whose total
> > length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
> > for a possible report index).  In theory a device could have a report
> > longer than that, but if there was such a thing we wouldn't handle it 
> > correctly anyway.
> > 
> > Reported-and-tested-by: syzbot+09ef48aa58261464b621@syzkaller.appspotmail.com
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > CC: <stable@vger.kernel.org>
> 
> Thanks for hunting this down Alan. Applied.

I just noticed this code:

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);
}

Does this indicate that the upper limit on a report length should 
really be HID_MAX_BUFFER_SIZE - 8 instead of HID_MAX_BUFFER_SIZE - 1?

Alan Stern
Jiri Kosina Dec. 13, 2019, 8:44 a.m. UTC | #3
On Wed, 11 Dec 2019, Alan Stern wrote:

> > > The syzbot fuzzer found a slab-out-of-bounds bug in the HID report
> > > handler.  The bug was caused by a report descriptor which included a
> > > field with size 12 bits and count 4899, for a total size of 7349
> > > bytes.
> > > 
> > > The usbhid driver uses at most a single-page 4-KB buffer for reports.
> > > In the test there wasn't any problem about overflowing the buffer,
> > > since only one byte was received from the device.  Rather, the bug
> > > occurred when the HID core tried to extract the data from the report
> > > fields, which caused it to try reading data beyond the end of the
> > > allocated buffer.
> > > 
> > > This patch fixes the problem by rejecting any report whose total
> > > length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
> > > for a possible report index).  In theory a device could have a report
> > > longer than that, but if there was such a thing we wouldn't handle it 
> > > correctly anyway.
> > > 
> > > Reported-and-tested-by: syzbot+09ef48aa58261464b621@syzkaller.appspotmail.com
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > CC: <stable@vger.kernel.org>
> > 
> > Thanks for hunting this down Alan. Applied.
> 
> I just noticed this code:
> 
> 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);
> }
> 
> Does this indicate that the upper limit on a report length should 
> really be HID_MAX_BUFFER_SIZE - 8 instead of HID_MAX_BUFFER_SIZE - 1?

As far as I remember, this is just very lousy way of properly rounding the 
size up (see 27ce405039bfe). So I believe HID_MAX_BUFFER_SIZE -1 is still 
functionally correct.

Thanks,

Patch
diff mbox series

Index: usb-devel/drivers/hid/hid-core.c
===================================================================
--- usb-devel.orig/drivers/hid/hid-core.c
+++ usb-devel/drivers/hid/hid-core.c
@@ -268,6 +268,12 @@  static int hid_add_field(struct hid_pars
 	offset = report->size;
 	report->size += parser->global.report_size * parser->global.report_count;
 
+	/* Total size check: Allow for possible report index byte */
+	if (report->size > (HID_MAX_BUFFER_SIZE - 1) << 3) {
+		hid_err(parser->device, "report is too long\n");
+		return -1;
+	}
+
 	if (!parser->local.usage_index) /* Ignore padding fields */
 		return 0;