From ddf7540e9c3a3d65739daa339c8838fa39cf2758 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Fri, 12 Jul 2013 11:01:02 +0200 Subject: HID: logitech-dj: use inlined helpers hid_hw_open/close Use the inlined helpers hid_hw_open/close instead of direct calls to ->ll_driver->open() and ->ll_driver->close(). Signed-off-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- drivers/hid/hid-logitech-dj.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/hid/hid-logitech-dj.c') diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 5207591a598c..db3192b24e6e 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -756,10 +756,10 @@ static int logi_dj_probe(struct hid_device *hdev, } /* This is enabling the polling urb on the IN endpoint */ - retval = hdev->ll_driver->open(hdev); + retval = hid_hw_open(hdev); if (retval < 0) { - dev_err(&hdev->dev, "%s:hdev->ll_driver->open returned " - "error:%d\n", __func__, retval); + dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n", + __func__, retval); goto llopen_failed; } @@ -776,7 +776,7 @@ static int logi_dj_probe(struct hid_device *hdev, return retval; logi_dj_recv_query_paired_devices_failed: - hdev->ll_driver->close(hdev); + hid_hw_close(hdev); llopen_failed: switch_to_dj_mode_fail: @@ -818,7 +818,7 @@ static void logi_dj_remove(struct hid_device *hdev) cancel_work_sync(&djrcv_dev->work); - hdev->ll_driver->close(hdev); + hid_hw_close(hdev); hid_hw_stop(hdev); /* I suppose that at this point the only context that can access -- cgit v1.2.3 From 27ce405039bfe6d3f4143415c638f56a3df77dca Mon Sep 17 00:00:00 2001 From: Jiri Kosina Date: Wed, 10 Jul 2013 19:56:27 +0200 Subject: 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 Acked-by: Gustavo Padovan Signed-off-by: Jiri Kosina --- 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(-) (limited to 'drivers/hid/hid-logitech-dj.c') 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) @@ -1144,6 +1145,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 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; diff --git a/include/linux/hid.h b/include/linux/hid.h index 0c48991b0402..acccdf4eb485 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -744,6 +744,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid); 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); diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c index 46c6a148f0b3..212980ff99b9 100644 --- a/net/bluetooth/hidp/core.c +++ b/net/bluetooth/hidp/core.c @@ -231,17 +231,21 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb) 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, -- cgit v1.2.3 From ce7373685ec33b94c293d9cfdb46daecb22db261 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 31 Jul 2013 17:17:58 -0400 Subject: HID: logitech-dj: Fix non-atomic kmalloc in logi_dj_ll_input_event() The ll_driver's .hidinput_input_event() method is called from atomic context [1]. Use GFP_ATOMIC for allocation of the synthesized hid report. BUG: sleeping function called from invalid context at /home/peter/src/kernels/next/mm/slub.c:941 in_atomic(): 1, irqs_disabled(): 1, pid: 2095, name: Xorg INFO: lockdep is turned off. irq event stamp: 1502178 hardirqs last enabled at (1502177): [] _raw_spin_unlock_irqrestore+0x65/0x80 hardirqs last disabled at (1502178): [] common_interrupt+0x6a/0x6f softirqs last enabled at (1501802): [] __do_softirq+0x183/0x420 softirqs last disabled at (1501799): [] irq_exit+0xb5/0xc0 CPU: 3 PID: 2095 Comm: Xorg Not tainted 3.11-next-20130725-xeon+lockdep #20130725 Hardware name: Dell Inc. Precision WorkStation T5400 /0RW203, BIOS A11 04/30/2012 ffffffff81a662e0 ffff8802adcf9ca8 ffffffff8177c330 0000000000000000 ffff8802a76d2440 ffff8802adcf9cd8 ffffffff810867d0 ffff8802a7ac8000 0000000000000010 00000000ffffffff 00000000000000d0 ffff8802adcf9d38 Call Trace: [] dump_stack+0x4f/0x84 [] __might_sleep+0x140/0x1f0 [] __kmalloc+0x6b/0x2e0 [] ? hid_alloc_report_buf+0x28/0x30 [hid] [] hid_alloc_report_buf+0x28/0x30 [hid] [] logi_dj_ll_input_event+0xb0/0x1b0 [hid_logitech_dj] [] input_handle_event+0x8e/0x540 [] ? input_inject_event+0x5d/0x220 [] input_inject_event+0x1c0/0x220 [] ? input_inject_event+0x44/0x220 [] ? might_fault+0xa0/0xb0 [] ? might_fault+0x57/0xb0 [] evdev_write+0xde/0x160 [] vfs_write+0xc8/0x1f0 [] SyS_write+0x55/0xa0 [] system_call_fastpath+0x16/0x1b Signed-off-by: Peter Hurley Signed-off-by: Jiri Kosina --- drivers/hid/hid-logitech-dj.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/hid/hid-logitech-dj.c') diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 4d792739dbd1..d318222c6315 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -591,7 +591,7 @@ static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type, } hid_set_field(field, offset, value); - data = hid_alloc_report_buf(field->report, GFP_KERNEL); + data = hid_alloc_report_buf(field->report, GFP_ATOMIC); if (!data) { dev_warn(&dev->dev, "failed to allocate report buf memory\n"); return -1; -- cgit v1.2.3