[ 004/110] HID: fix data access in implement()

[Greg Kroah-Hartman]

3.10-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Jiri Kosina <jkosina@xxxxxxx>

commit 27ce405039bfe6d3f4143415c638f56a3df77dca upstream.

implement() is setting bytes in LE data stream. In case the data is not
aligned to 64bits, it reads past the allocated buffer. It doesn't really
change any value there (it's properly bitmasked), but in case that this
read past the boundary hits a page boundary, pagefault happens when
accessing 64bits of 'x' in implement(), and kernel oopses.

This happens much more often when numbered reports are in use, as the
initial 8bit skip in the buffer makes the whole process work on values
which are not aligned to 64bits.

This problem dates back to attempts in 2005 and 2006 to make implement()
and extract() as generic as possible, and even back then the problem
was realized by Adam Kroperlin, but falsely assumed to be impossible
to cause any harm:

  http://www.mail-archive.com/linux-usb-devel@xxxxxxxxxxxxxxxxxxxxx/msg47690.html

I have made several attempts at fixing it "on the spot" directly in
implement(), but the results were horrible; the special casing for processing
last 64bit chunk and switching to different math makes it unreadable mess.

I therefore took a path to allocate a few bytes more which will never make
it into final report, but are there as a cushion for all the 64bit math
operations happening in implement() and extract().

All callers of hid_output_report() are converted at the same time to allocate
the buffer by newly introduced hid_alloc_report_buf() helper.

Bruno noticed that the whole raw_size test can be dropped as well, as
hid_alloc_report_buf() makes sure that the buffer is always of a proper
size.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
Acked-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

---
 drivers/hid/hid-core.c            |   19 ++++++++++++++++++-
 drivers/hid/hid-logitech-dj.c     |   12 ++++++++++--
 drivers/hid/hid-picolcd_debugfs.c |   23 ++++++++++++-----------
 drivers/hid/usbhid/hid-core.c     |    5 ++---
 include/linux/hid.h               |    1 +
 net/bluetooth/hidp/core.c         |   14 +++++++++-----
 6 files changed, 52 insertions(+), 22 deletions(-)

--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1188,7 +1188,8 @@ static void hid_output_field(const struc
 }
 
 /*
- * Create a report.
+ * Create a report. 'data' has to be allocated using
+ * hid_alloc_report_buf() so that it has proper size.
  */
 
 void hid_output_report(struct hid_report *report, __u8 *data)
@@ -1205,6 +1206,22 @@ void hid_output_report(struct hid_report
 EXPORT_SYMBOL_GPL(hid_output_report);
 
 /*
+ * Allocator for buffer that is going to be passed to hid_output_report()
+ */
+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
+	 */
+
+	int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
+
+	return kmalloc(len, flags);
+}
+EXPORT_SYMBOL_GPL(hid_alloc_report_buf);
+
+/*
  * Set a field value. The report this field belongs to has to be
  * created and transferred to the device, to set this value in the
  * device.
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -574,7 +574,7 @@ static int logi_dj_ll_input_event(struct
 
 	struct hid_field *field;
 	struct hid_report *report;
-	unsigned char data[8];
+	unsigned char *data;
 	int offset;
 
 	dbg_hid("%s: %s, type:%d | code:%d | value:%d\n",
@@ -590,6 +590,13 @@ static int logi_dj_ll_input_event(struct
 		return -1;
 	}
 	hid_set_field(field, offset, value);
+
+	data = hid_alloc_report_buf(field->report, GFP_KERNEL);
+	if (!data) {
+		dev_warn(&dev->dev, "failed to allocate report buf memory\n");
+		return -1;
+	}
+
 	hid_output_report(field->report, &data[0]);
 
 	output_report_enum = &dj_rcv_hiddev->report_enum[HID_OUTPUT_REPORT];
@@ -600,8 +607,9 @@ static int logi_dj_ll_input_event(struct
 
 	hid_hw_request(dj_rcv_hiddev, report, HID_REQ_SET_REPORT);
 
-	return 0;
+	kfree(data);
 
+	return 0;
 }
 
 static int logi_dj_ll_start(struct hid_device *hid)
--- a/drivers/hid/hid-picolcd_debugfs.c
+++ b/drivers/hid/hid-picolcd_debugfs.c
@@ -394,7 +394,7 @@ static void dump_buff_as_hex(char *dst,
 void picolcd_debug_out_report(struct picolcd_data *data,
 		struct hid_device *hdev, struct hid_report *report)
 {
-	u8 raw_data[70];
+	u8 *raw_data;
 	int raw_size = (report->size >> 3) + 1;
 	char *buff;
 #define BUFF_SZ 256
@@ -407,20 +407,20 @@ void picolcd_debug_out_report(struct pic
 	if (!buff)
 		return;
 
-	snprintf(buff, BUFF_SZ, "\nout report %d (size %d) =  ",
-			report->id, raw_size);
-	hid_debug_event(hdev, buff);
-	if (raw_size + 5 > sizeof(raw_data)) {
+	raw_data = hid_alloc_report_buf(report, GFP_ATOMIC);
+	if (!raw_data) {
 		kfree(buff);
-		hid_debug_event(hdev, " TOO BIG\n");
 		return;
-	} else {
-		raw_data[0] = report->id;
-		hid_output_report(report, raw_data);
-		dump_buff_as_hex(buff, BUFF_SZ, raw_data, raw_size);
-		hid_debug_event(hdev, buff);
 	}
 
+	snprintf(buff, BUFF_SZ, "\nout report %d (size %d) =  ",
+			report->id, raw_size);
+	hid_debug_event(hdev, buff);
+	raw_data[0] = report->id;
+	hid_output_report(report, raw_data);
+	dump_buff_as_hex(buff, BUFF_SZ, raw_data, raw_size);
+	hid_debug_event(hdev, buff);
+
 	switch (report->id) {
 	case REPORT_LED_STATE:
 		/* 1 data byte with GPO state */
@@ -644,6 +644,7 @@ void picolcd_debug_out_report(struct pic
 		break;
 	}
 	wake_up_interruptible(&hdev->debug_wait);
+	kfree(raw_data);
 	kfree(buff);
 }
 
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -535,7 +535,6 @@ static void __usbhid_submit_report(struc
 {
 	int head;
 	struct usbhid_device *usbhid = hid->driver_data;
-	int len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 
 	if ((hid->quirks & HID_QUIRK_NOGET) && dir == USB_DIR_IN)
 		return;
@@ -546,7 +545,7 @@ static void __usbhid_submit_report(struc
 			return;
 		}
 
-		usbhid->out[usbhid->outhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		usbhid->out[usbhid->outhead].raw_report = hid_alloc_report_buf(report, GFP_ATOMIC);
 		if (!usbhid->out[usbhid->outhead].raw_report) {
 			hid_warn(hid, "output queueing failed\n");
 			return;
@@ -595,7 +594,7 @@ static void __usbhid_submit_report(struc
 	}
 
 	if (dir == USB_DIR_OUT) {
-		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		usbhid->ctrl[usbhid->ctrlhead].raw_report = hid_alloc_report_buf(report, GFP_ATOMIC);
 		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
 			hid_warn(hid, "control queueing failed\n");
 			return;
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -746,6 +746,7 @@ struct hid_field *hidinput_get_led_field
 unsigned int hidinput_count_leds(struct hid_device *hid);
 __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
 void hid_output_report(struct hid_report *report, __u8 *data);
+u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -231,17 +231,21 @@ static void hidp_input_report(struct hid
 
 static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
 {
-	unsigned char buf[32], hdr;
-	int rsize;
+	unsigned char hdr;
+	u8 *buf;
+	int rsize, ret;
 
-	rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
-	if (rsize > sizeof(buf))
+	buf = hid_alloc_report_buf(report, GFP_ATOMIC);
+	if (!buf)
 		return -EIO;
 
 	hid_output_report(report, buf);
 	hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
 
-	return hidp_send_intr_message(session, hdr, buf, rsize);
+	ret = hidp_send_intr_message(session, hdr, buf, rsize);
+
+	kfree(buf);
+	return ret;
 }
 
 static int hidp_get_raw_report(struct hid_device *hid,


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/