summaryrefslogtreecommitdiffstats
path: root/drivers/hid
diff options
context:
space:
mode:
authorJiri Kosina <jkosina@suse.cz>2013-07-10 19:56:27 +0200
committerJiri Kosina <jkosina@suse.cz>2013-07-22 16:16:40 +0200
commit27ce405039bfe6d3f4143415c638f56a3df77dca (patch)
tree5419dac5ff32335c81bb1d753a2c0df181ed44c7 /drivers/hid
parent0adb9c2c5ed42f199cb2a630c37d18dee385fae2 (diff)
downloadlinux-27ce405039bfe6d3f4143415c638f56a3df77dca.tar.bz2
HID: fix data access in implement()
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@lists.sourceforge.net/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@redhat.com> Acked-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Diffstat (limited to 'drivers/hid')
-rw-r--r--drivers/hid/hid-core.c19
-rw-r--r--drivers/hid/hid-logitech-dj.c12
-rw-r--r--drivers/hid/hid-picolcd_debugfs.c23
-rw-r--r--drivers/hid/usbhid/hid-core.c5
4 files changed, 42 insertions, 17 deletions
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b0f2f459f59b..a1b248cea5b0 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1128,7 +1128,8 @@ static void hid_output_field(const struct hid_device *hid,
}
/*
- * 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)
@@ -1145,6 +1146,22 @@ void hid_output_report(struct hid_report *report, __u8 *data)
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.
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 5207591a598c..4d792739dbd1 100644
--- 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 input_dev *dev, unsigned int type,
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 input_dev *dev, unsigned int type,
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 input_dev *dev, unsigned int type,
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)
diff --git a/drivers/hid/hid-picolcd_debugfs.c b/drivers/hid/hid-picolcd_debugfs.c
index 59ab8e157e6b..024cdf3c2297 100644
--- 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, size_t dst_sz, const u8 *data,
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 picolcd_data *data,
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 picolcd_data *data,
break;
}
wake_up_interruptible(&hdev->debug_wait);
+ kfree(raw_data);
kfree(buff);
}
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 99418285222c..ada164e1b3a1 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -535,7 +535,6 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
{
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(struct hid_device *hid, struct hid_report *re
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(struct hid_device *hid, struct hid_report *re
}
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;