[PATCH 11/25] tty: synclinkmp: Change return type to void
Since tty_port_install() always returns 0, the return type has changed to void. Now apply this and remove the obsolete error check. Signed-off-by: Jaejoong Kim --- drivers/tty/synclinkmp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c index 1e4d5b9..2d99a5b 100644 --- a/drivers/tty/synclinkmp.c +++ b/drivers/tty/synclinkmp.c @@ -734,8 +734,9 @@ static int install(struct tty_driver *driver, struct tty_struct *tty) } tty->driver_data = info; + tty_port_install(&info->port, driver, tty); - return tty_port_install(&info->port, driver, tty); + return 0; } /* Called when a port is opened. Init and enable port. -- 2.7.4
[PATCH 12/25] tty: vt: Change return type to void
Since tty_port_install() always returns 0, the return type has changed to void. Now apply this and remove the obsolete error check. Signed-off-by: Jaejoong Kim --- drivers/tty/vt/vt.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 5f1183b..cc72254 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -3222,10 +3222,7 @@ static int con_install(struct tty_driver *driver, struct tty_struct *tty) goto unlock; } - ret = tty_port_install(&vc->port, driver, tty); - if (ret) - goto unlock; - + tty_port_install(&vc->port, driver, tty); tty->driver_data = vc; vc->port.tty = tty; -- 2.7.4
[PATCH 06/25] tty: hvc: hvcs: Change return type to void
Since tty_port_install() always returns 0, the return type has changed to void. Now apply this and remove the obsolete error check. Signed-off-by: Jaejoong Kim --- drivers/tty/hvc/hvcs.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c index cb4db1b..4dfa70c 100644 --- a/drivers/tty/hvc/hvcs.c +++ b/drivers/tty/hvc/hvcs.c @@ -1140,16 +1140,10 @@ static int hvcs_install(struct tty_driver *driver, struct tty_struct *tty) goto err_put; } - retval = tty_port_install(&hvcsd->port, driver, tty); - if (retval) - goto err_irq; + tty_port_install(&hvcsd->port, driver, tty); return 0; -err_irq: - spin_lock_irqsave(&hvcsd->lock, flags); - vio_disable_interrupts(hvcsd->vdev); - spin_unlock_irqrestore(&hvcsd->lock, flags); - free_irq(irq, hvcsd); + err_put: tty_port_put(&hvcsd->port); -- 2.7.4
Re: [PATCH] gitignore: add *.gcda files
2018-01-17 22:07 GMT+09:00 Peter Oberparleiter : > On 09.01.2018 06:17, Jaejoong Kim wrote: >> 2017-12-20 16:09 GMT+09:00 Jaejoong Kim : >>> Ignore the *.gcda files generated by gcov >>> >>> Signed-off-by: Jaejoong Kim >>> --- >>> .gitignore | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/.gitignore b/.gitignore >>> index 0c39aa2..580ef7c 100644 >>> --- a/.gitignore >>> +++ b/.gitignore >>> @@ -39,6 +39,7 @@ Module.symvers >>> *.dwo >>> *.su >>> *.c.[012]*.* >>> +*.gcda > > The gcov-kernel mechanism generates .gcda files for kernel code only as > virtual files in debugfs, and not in the kernel source tree. You are right. The .gcda files is from debugfs and I copied it to the kernel source tree. Sorry for the bother you. Please ignore this patch file. Thanks jaejoong What source > of .gcda files would be covered by this .gitignore change? > >>> >>> # >>> # Top-level generic files >>> -- >>> 2.7.4 >>> > > -- > Peter Oberparleiter > Linux on z Systems Development - IBM Germany >
Re: [PATCH] gitignore: add *.gcda files
Hi, Peter Could you check this patch? Thanks, Jaejoong 2017-12-20 16:09 GMT+09:00 Jaejoong Kim : > Ignore the *.gcda files generated by gcov > > Signed-off-by: Jaejoong Kim > --- > .gitignore | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.gitignore b/.gitignore > index 0c39aa2..580ef7c 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -39,6 +39,7 @@ Module.symvers > *.dwo > *.su > *.c.[012]*.* > +*.gcda > > # > # Top-level generic files > -- > 2.7.4 >
[PATCH] gitignore: add *.gcda files
Ignore the *.gcda files generated by gcov Signed-off-by: Jaejoong Kim --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 0c39aa2..580ef7c 100644 --- a/.gitignore +++ b/.gitignore @@ -39,6 +39,7 @@ Module.symvers *.dwo *.su *.c.[012]*.* +*.gcda # # Top-level generic files -- 2.7.4
Re: [PATCH] media: av7110: switch to useing timer_setup()
Hi,
[PATCH] media: av7110: switch to useing timer_setup()
^^^
typo error.
2017-10-25 9:40 GMT+09:00 Dmitry Torokhov :
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
>
> Also stop poking into input core internals and override its autorepeat
> timer function. I am not sure why we have such convoluted autorepeat
> handling in this driver instead of letting input core handle autorepeat,
> but this preserves current behavior of allowing controlling autorepeat
> delay and forcing autorepeat period to be whatever the hardware has.
>
> Signed-off-by: Dmitry Torokhov
> ---
>
> Note that this has not been tested on the hardware. But it should
> compile, so I have that going for me.
>
> drivers/media/pci/ttpci/av7110.h| 4 ++--
> drivers/media/pci/ttpci/av7110_ir.c | 40
> +
> 2 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/pci/ttpci/av7110.h
> b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..0aa3c6f01853 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -80,10 +80,11 @@ struct av7110;
>
> /* infrared remote control */
> struct infrared {
> - u16 key_map[256];
> + u16 key_map[256];
> struct input_dev*input_dev;
> charinput_phys[32];
> struct timer_list keyup_timer;
> + unsigned long keydown_time;
> struct tasklet_struct ir_tasklet;
> void(*ir_handler)(struct av7110 *av7110, u32
> ircom);
> u32 ir_command;
> @@ -93,7 +94,6 @@ struct infrared {
> u8 inversion;
> u16 last_key;
> u16 last_toggle;
> - u8 delay_timer_finished;
> };
>
>
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c
> b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..b602e64b3412 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -84,9 +84,9 @@ static u16 default_key_map [256] = {
>
>
> /* key-up timer */
> -static void av7110_emit_keyup(unsigned long parm)
> +static void av7110_emit_keyup(struct timer_list *t)
> {
> - struct infrared *ir = (struct infrared *) parm;
> + struct infrared *ir = from_timer(ir, keyup_timer, t);
>
> if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> return;
> @@ -152,19 +152,20 @@ static void av7110_emit_key(unsigned long parm)
> return;
> }
>
> - if (timer_pending(&ir->keyup_timer)) {
> - del_timer(&ir->keyup_timer);
> + if (del_timer(&ir->keyup_timer)) {
> if (ir->last_key != keycode || toggle != ir->last_toggle) {
> - ir->delay_timer_finished = 0;
> + ir->keydown_time = jiffies;
> input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
> input_event(ir->input_dev, EV_KEY, keycode, 1);
> input_sync(ir->input_dev);
> - } else if (ir->delay_timer_finished) {
> + } else if (time_after(jiffies, ir->keydown_time +
> + msecs_to_jiffies(
> + ir->input_dev->rep[REP_PERIOD]))) {
> input_event(ir->input_dev, EV_KEY, keycode, 2);
> input_sync(ir->input_dev);
> }
> } else {
> - ir->delay_timer_finished = 0;
> + ir->keydown_time = jiffies;
> input_event(ir->input_dev, EV_KEY, keycode, 1);
> input_sync(ir->input_dev);
> }
> @@ -172,9 +173,7 @@ static void av7110_emit_key(unsigned long parm)
> ir->last_key = keycode;
> ir->last_toggle = toggle;
>
> - ir->keyup_timer.expires = jiffies + UP_TIMEOUT;
> - add_timer(&ir->keyup_timer);
> -
> + mod_timer(&ir->keyup_timer, jiffies + UP_TIMEOUT);
> }
>
>
> @@ -184,12 +183,19 @@ static void input_register_keys(struct infrared *ir)
> int i;
>
> set_bit(EV_KEY, ir->input_dev->evbit);
> - set_bit(EV_REP, ir->input_dev->evbit);
> set_bit(EV_MSC, ir->input_dev->evbit);
>
> set_bit(MSC_RAW, ir->input_dev->mscbit);
> set_bit(MSC_SCAN, ir->input_dev->mscbit);
>
> + set_bit(EV_REP, ir->input_dev->evbit);
> + /*
> +* By setting the delay before registering input device we
> +* indicate that we will be implementing the autorepeat
> +* ourselves.
> +*/
> + ir->input_dev->rep[REP_DELAY] = 250;
> +
> memset(ir->input_dev->keybit, 0, sizeof(ir->i
Re: [PATCH v2] HID: usbhid: fix out-of-bounds bug
Hi,
To. Jiri, Alan,
Could you please review this patch?
To. Andey,
Could you please test with this patch for KASAN OOB error?
Thanks, jaejoong
2017-09-28 19:16 GMT+09:00 Jaejoong Kim :
> The hid descriptor identifies the length and type of subordinate
> descriptors for a device. If the received hid descriptor is smaller than
> the size of the struct hid_descriptor, it is possible to cause
> out-of-bounds.
>
> In addition, if bNumDescriptors of the hid descriptor have an incorrect
> value, this can also cause out-of-bounds while approaching hdesc->desc[n].
>
> So check the size of hid descriptor and bNumDescriptors.
>
> BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
> Read of size 1 at addr 88006c5f8edf by task kworker/1:2/1261
>
> CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
> 4.14.0-rc1-42251-gebb2c2437d80 #169
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> __dump_stack lib/dump_stack.c:16
> dump_stack+0x292/0x395 lib/dump_stack.c:52
> print_address_description+0x78/0x280 mm/kasan/report.c:252
> kasan_report_error mm/kasan/report.c:351
> kasan_report+0x22f/0x340 mm/kasan/report.c:409
> __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
> usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
> hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
> usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
> usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
> really_probe drivers/base/dd.c:413
> driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> device_add+0xd0b/0x1660 drivers/base/core.c:1835
> usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
> generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
> usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
> really_probe drivers/base/dd.c:413
> driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> device_add+0xd0b/0x1660 drivers/base/core.c:1835
> usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
> hub_port_connect drivers/usb/core/hub.c:4903
> hub_port_connect_change drivers/usb/core/hub.c:5009
> port_event drivers/usb/core/hub.c:5115
> hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
> process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
> worker_thread+0x221/0x1850 kernel/workqueue.c:2253
> kthread+0x3a1/0x470 kernel/kthread.c:231
> ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
>
> Reported-by: Andrey Konovalov
> Signed-off-by: Jaejoong Kim
> ---
>
> Changes in v2:
> - write a new commit message because orginal version is wrong approach
> - add check hid descriptor size
> - get proper value for bNumDescriptors as suggested by Alan Stern
> - fix the Reported-by
>
> drivers/hid/usbhid/hid-core.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..045b5da 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
> unsigned int rsize = 0;
> char *rdesc;
> int ret, n;
> + int num_descriptors;
> + size_t offset = offsetof(struct hid_descriptor, desc);
>
> quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
> le16_to_cpu(dev->descriptor.idProduct));
> @@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
> return -ENODEV;
> }
>
> + if (hdesc->bLength < sizeof(struct hid_descriptor)) {
> + dbg_hid("hid descriptor is too short\n");
> + return -EINVAL;
> + }
> +
> hid->version = le16_to_cpu(hdesc-
Re: [PATCH 1/2] USB: serial: console: fix use-after-free on disconnect
Hi
2017-10-04 18:01 GMT+09:00 Johan Hovold :
> A clean-up patch removing removing two redundant NULL-checks from the
^^
The word 'removing' was written twice. :)
> console disconnect handler inadvertently also removed a third check.
> This could lead to the struct usb_serial being prematurely freed by the
> console code when a driver accepts but does not register any ports for
> an interface which also lacks endpoint descriptors.
>
> Fixes: 0e517c93dc02 ("USB: serial: console: clean up sanity checks")
> Cc: stable # 4.11
> Reported-by: Andrey Konovalov
> Signed-off-by: Johan Hovold
> ---
> drivers/usb/serial/console.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
> index fdf89800ebc3..ed8ba3ef5c79 100644
> --- a/drivers/usb/serial/console.c
> +++ b/drivers/usb/serial/console.c
> @@ -265,7 +265,7 @@ static struct console usbcons = {
>
> void usb_serial_console_disconnect(struct usb_serial *serial)
> {
> - if (serial->port[0] == usbcons_info.port) {
> + if (serial->port[0] && serial->port[0] == usbcons_info.port) {
> usb_serial_console_exit();
> usb_serial_put(serial);
> }
> --
> 2.14.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] HID: usbhid: fix out-of-bounds bug
The hid descriptor identifies the length and type of subordinate
descriptors for a device. If the received hid descriptor is smaller than
the size of the struct hid_descriptor, it is possible to cause
out-of-bounds.
In addition, if bNumDescriptors of the hid descriptor have an incorrect
value, this can also cause out-of-bounds while approaching hdesc->desc[n].
So check the size of hid descriptor and bNumDescriptors.
BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
Read of size 1 at addr 88006c5f8edf by task kworker/1:2/1261
CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
4.14.0-rc1-42251-gebb2c2437d80 #169
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x292/0x395 lib/dump_stack.c:52
print_address_description+0x78/0x280 mm/kasan/report.c:252
kasan_report_error mm/kasan/report.c:351
kasan_report+0x22f/0x340 mm/kasan/report.c:409
__asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
really_probe drivers/base/dd.c:413
driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
__device_attach_driver+0x230/0x290 drivers/base/dd.c:653
bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
__device_attach+0x26e/0x3d0 drivers/base/dd.c:710
device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
device_add+0xd0b/0x1660 drivers/base/core.c:1835
usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
really_probe drivers/base/dd.c:413
driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
__device_attach_driver+0x230/0x290 drivers/base/dd.c:653
bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
__device_attach+0x26e/0x3d0 drivers/base/dd.c:710
device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
device_add+0xd0b/0x1660 drivers/base/core.c:1835
usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
hub_port_connect drivers/usb/core/hub.c:4903
hub_port_connect_change drivers/usb/core/hub.c:5009
port_event drivers/usb/core/hub.c:5115
hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
worker_thread+0x221/0x1850 kernel/workqueue.c:2253
kthread+0x3a1/0x470 kernel/kthread.c:231
ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
Reported-by: Andrey Konovalov
Signed-off-by: Jaejoong Kim
---
Changes in v2:
- write a new commit message because orginal version is wrong approach
- add check hid descriptor size
- get proper value for bNumDescriptors as suggested by Alan Stern
- fix the Reported-by
drivers/hid/usbhid/hid-core.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..045b5da 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
unsigned int rsize = 0;
char *rdesc;
int ret, n;
+ int num_descriptors;
+ size_t offset = offsetof(struct hid_descriptor, desc);
quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
le16_to_cpu(dev->descriptor.idProduct));
@@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
return -ENODEV;
}
+ if (hdesc->bLength < sizeof(struct hid_descriptor)) {
+ dbg_hid("hid descriptor is too short\n");
+ return -EINVAL;
+ }
+
hid->version = le16_to_cpu(hdesc->bcdHID);
hid->country = hdesc->bCountryCode;
- for (n = 0; n < hdesc->bNumDescriptors; n++)
+ num_descriptors = min_t(int, hdesc->bNumDescriptors,
+ (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
+
+ for (n = 0; n < num_descriptors; n++)
if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
--
2.7.4
Re: [PATCH] HID: usbhid: fix out-of-bounds bug
2017-09-27 23:29 GMT+09:00 Alan Stern : > On Wed, 27 Sep 2017, Michel Hermier wrote: > >> Le 27 sept. 2017 07:42, "Alan Stern" a écrit : > >> > - for (n = 0; n < hdesc->bNumDescriptors; n++) >> > + num_descriptors = min_t(int, hdesc->bNumDescriptors, >> > + (hdesc->bLength - 6) / 3); >> > + for (n = 0; n < num_descriptors; n++) >> > if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT) >> > rsize = le16_to_cpu(hdesc->desc[n]. >> wDescriptorLength); >> >> Yes, this is a lot better. OK. >> >> >> Is it possible to explicit the magic number 6 and 3 in the code. Currently, >> it looks like it comes from no where. I gree with you. > > Yes, it is possible. The 6 is equal to > > offsetof(struct hid_descriptor, desc) > > and the 3 is equal to > > sizeof(struct hid_class_descriptor) > > (at least, I think it is -- the structure is marked as packed so its > size should be 3). > > In this case I found the numbers to be more readable, but other people > may have different opinions. I will post V2 shortly. > >> I'm also wondering if this change will not affect some devices in the wild, >> by rejecting hid descriptors with num descriptors == 0 ? > > It's possible, but I doubt it. If such devices do exist, they should > never have worked in the first place. Certainly they would generate > warnings or errors during enumeration because of their invalid > descriptors. > > Alan Stern > Jaejoong
Re: [PATCH] HID: usbhid: fix out-of-bounds bug
Hi, Alan,
Thanks for the review.
2017-09-26 23:18 GMT+09:00 Alan Stern :
> On Tue, 26 Sep 2017, Jaejoong Kim wrote:
>
>> The starting address of the hid descriptor is obtained via
>> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
>> is possible to access the wrong address. So, before accessing the hid
>> descriptor, we need to check the entire size through the bLength field.
>>
>> It also shows how many class descriptors it has through the bNumDescriptors
>> of the hid descriptor. Assuming that the connected hid descriptor has two
>> class descriptors(report and physical descriptors), the code below can
>> cause OOB because hdesc->desc is an array of size 1.
>>
>> for (n = 0; n < hdesc->bNumDescriptors; n++)
>> if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>> rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>>
>> Since we know the starting address of the hid descriptor and the value of
>> the bNumDescriptors is variable, we directly access the buffer containing
>> the hid descriptor without usbing hdesc->desc to obtain the size of the
>> report descriptor.
>
> I do not like this patch at all.
>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 089bad8..7bad173 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
>> struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>> struct usb_host_interface *interface = intf->cur_altsetting;
>> struct usb_device *dev = interface_to_usbdev (intf);
>> - struct hid_descriptor *hdesc;
>> + unsigned char *buffer = intf->altsetting->extra;
>> + int buflen = intf->altsetting->extralen;
>> + int length;
>> u32 quirks = 0;
>> unsigned int rsize = 0;
>> char *rdesc;
>> int ret, n;
>>
>> + if (!buffer) {
>> + dbg_hid("class descriptor not present\n");
>> + return -ENODEV;
>> + }
>> +
>> quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>> le16_to_cpu(dev->descriptor.idProduct));
>>
>> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
>> quirks |= HID_QUIRK_NOGET;
>> }
>>
>> - if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
>> - (!interface->desc.bNumEndpoints ||
>> - usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID,
>> &hdesc))) {
>> - dbg_hid("class descriptor not present\n");
>> - return -ENODEV;
>> - }
>> + while (buflen > 2) {
>> + length = buffer[0];
>> + if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
>> + goto next_desc;
>
> It's silly to duplicate the code that is already present in
> usb_get_extra_descriptor(). There's no reason to avoid using that
> function here.
To be honest, I was fooled. You are right. That is a duplicate code
with usb_get_extra_descriptor().
>
>> - hid->version = le16_to_cpu(hdesc->bcdHID);
>> - hid->country = hdesc->bCountryCode;
>> + if (buffer[1] == HID_DT_HID) {
>> + hid->version = get_unaligned_le16(&buffer[2]);
>> + hid->country = buffer[4];
>
> It's also silly not to use the preformatted hid_descriptor structure and
> instead put error-prone byte offsets directly into the code.
Right.
>
>> - for (n = 0; n < hdesc->bNumDescriptors; n++)
>> - if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>> - rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>> + for (n = 0; n < buffer[5]; n++) {
>> + /* we are just interested in report descriptor
>> */
>> + if (buffer[6+3*n] != HID_DT_REPORT)
>> + continue;
>> + rsize = get_unaligned_le16(&buffer[7+3*n]);
>
> Finally, this patch doesn't fix the actual problem! You don't check
> here whether 8+3*n >= length.
>
> This whole thing could be fixed with a much smaller change to the
> original code. Just do something like this:
>
> num_descriptors = min_t(int, hdesc->bNumDescriptors
[PATCH] HID: usbhid: fix out-of-bounds bug
ook mm/slub.c:1412
slab_free mm/slub.c:2988
kfree+0xf6/0x2f0 mm/slub.c:3919
free_rb_tree_fname+0x8a/0xe0 fs/ext4/dir.c:402
ext4_htree_free_dir_info fs/ext4/dir.c:424
ext4_release_dir+0x49/0x70 fs/ext4/dir.c:622
__fput+0x33e/0x800 fs/file_table.c:210
fput+0x1a/0x20 fs/file_table.c:244
task_work_run+0x1af/0x280 kernel/task_work.c:112
tracehook_notify_resume ./include/linux/tracehook.h:191
exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
prepare_exit_to_usermode arch/x86/entry/common.c:197
syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238
The buggy address belongs to the object at 88006c5f8ea0
which belongs to the cache kmalloc-64 of size 64
The buggy address is located 63 bytes inside of
64-byte region [88006c5f8ea0, 88006c5f8ee0)
The buggy address belongs to the page:
page:ea0001b17e00 count:1 mapcount:0 mapping: (null)
index:0x0
flags: 0x1000100(slab)
raw: 01000100 0001002a002a
raw: ea0001a83000 00150015 88006c403800
page dumped because: kasan: bad access detected
Memory state around the buggy address:
88006c5f8d80: fb fb fb fb fb fb fb fb fc fc fc fc 00 00 00 00
88006c5f8e00: 00 fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>88006c5f8e80: fc fc fc fc 00 00 00 00 00 00 00 07 fc fc fc fc
^
88006c5f8f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
88006c5f8f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
==
Reported-by: Alexander Potapenko
Signed-off-by: Jaejoong Kim
---
drivers/hid/usbhid/hid-core.c | 39 +++
include/linux/hid.h | 2 ++
2 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..7bad173 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
struct usb_interface *intf = to_usb_interface(hid->dev.parent);
struct usb_host_interface *interface = intf->cur_altsetting;
struct usb_device *dev = interface_to_usbdev (intf);
- struct hid_descriptor *hdesc;
+ unsigned char *buffer = intf->altsetting->extra;
+ int buflen = intf->altsetting->extralen;
+ int length;
u32 quirks = 0;
unsigned int rsize = 0;
char *rdesc;
int ret, n;
+ if (!buffer) {
+ dbg_hid("class descriptor not present\n");
+ return -ENODEV;
+ }
+
quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
le16_to_cpu(dev->descriptor.idProduct));
@@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
quirks |= HID_QUIRK_NOGET;
}
- if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
- (!interface->desc.bNumEndpoints ||
-usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID,
&hdesc))) {
- dbg_hid("class descriptor not present\n");
- return -ENODEV;
- }
+ while (buflen > 2) {
+ length = buffer[0];
+ if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
+ goto next_desc;
- hid->version = le16_to_cpu(hdesc->bcdHID);
- hid->country = hdesc->bCountryCode;
+ if (buffer[1] == HID_DT_HID) {
+ hid->version = get_unaligned_le16(&buffer[2]);
+ hid->country = buffer[4];
- for (n = 0; n < hdesc->bNumDescriptors; n++)
- if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
- rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
+ for (n = 0; n < buffer[5]; n++) {
+ /* we are just interested in report descriptor
*/
+ if (buffer[6+3*n] != HID_DT_REPORT)
+ continue;
+ rsize = get_unaligned_le16(&buffer[7+3*n]);
+ }
+ }
+
+next_desc:
+ buflen -= length;
+ buffer += length;
+ }
if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
dbg_hid("weird size of report descriptor (%u)\n", rsize);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ab05a86..2d53c0f 100644
--- a/
Re: usb/hid: slab-out-of-bounds read in usbhid_parse
Hi Alan 2017-09-21 0:50 GMT+09:00 Alan Stern : > On Wed, 20 Sep 2017, Kim Jaejoong wrote: > >> To. usb & input guys. >> >> While dig this report, i was wondering about bNumDescriptors in HID >> descriptor. >> HID document from usb.org said, 'this number must be at least one (1) >> as a Report descriptor will always be present.' >> >> There is no mention of the order of class descriptors. Suppose you >> have a HID device with a report descriptor and a physical descriptor. >> >> If you have the following hid descriptor in this case, >> HID descriptor >>bLength: 12 >>bDescriptor Type: HID >>.. skip >>bNumDescriptors: 2 >>bDescriptorType: physical >>bDescriptorLength: any >>bDescriptorType: Report >>bDescriptorLength: any >> >> If the order of the report descriptor is the second as above, >> usbhid_parse () will fail because my patch is only check the first >> bDescriptor Type. >> But If the order of the report descriptor is always first, there is no >> problem. How do you think this? > > The descriptors can appear in any order. You should not assume that > the report descriptor will always come first. Thanks for clarifying. I will resend patch with modification. Jaejoong > > Alan Stern >
[PATCH v2 2/2] HID: hiddev: reallocate hiddev's minor number
We need to store the minor number each drivers. In case of hidraw, the
minor number is stored stores in struct hidraw. But hiddev's minor is
located in struct hid_device.
The hid-core driver announces a kernel message which driver is loaded when
HID device connected, but hiddev's minor number is always zero. To proper
display hiddev's minor number, we need to store the minor number asked from
usb core and do some refactoring work (move from hiddev.c to hiddev.h) to
access hiddev in hid-core.
Reviewed-by: Benjamin Tissoires
Signed-off-by: Jaejoong Kim
---
Chanes in v2:
- rollback struct hiddev_list exported
- squash this patch in a seperate patch
---
drivers/hid/hid-core.c | 2 +-
drivers/hid/usbhid/hiddev.c | 15 ---
include/linux/hid.h | 1 -
include/linux/hiddev.h | 11 +++
4 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index e9e87d3..1a0b910 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1695,7 +1695,7 @@ int hid_connect(struct hid_device *hdev, unsigned int
connect_mask)
len += sprintf(buf + len, "input");
if (hdev->claimed & HID_CLAIMED_HIDDEV)
len += sprintf(buf + len, "%shiddev%d", len ? "," : "",
- hdev->minor);
+ ((struct hiddev *)hdev->hiddev)->minor);
if (hdev->claimed & HID_CLAIMED_HIDRAW)
len += sprintf(buf + len, "%shidraw%d", len ? "," : "",
((struct hidraw *)hdev->hidraw)->minor);
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 700145b..27721c3 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -44,17 +44,7 @@
#define HIDDEV_MINOR_BASE 96
#define HIDDEV_MINORS 16
#endif
-#define HIDDEV_BUFFER_SIZE 2048
-
-struct hiddev {
- int exist;
- int open;
- struct mutex existancelock;
- wait_queue_head_t wait;
- struct hid_device *hid;
- struct list_head list;
- spinlock_t list_lock;
-};
+#define HIDDEV_BUFFER_SIZE 2048
struct hiddev_list {
struct hiddev_usage_ref buffer[HIDDEV_BUFFER_SIZE];
@@ -910,6 +900,9 @@ int hiddev_connect(struct hid_device *hid, unsigned int
force)
kfree(hiddev);
return -1;
}
+
+ hiddev->minor = usbhid->intf->minor;
+
return 0;
}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 28f38e2b8..643c017 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -541,7 +541,6 @@ struct hid_device {
/* device report descriptor */
struct list_head inputs;/* The
list of inputs */
void *hiddev; /* The
hiddev structure */
void *hidraw;
- int minor; /*
Hiddev minor number */
int open; /* is
the device open by anyone? */
char name[128]; /*
Device name */
diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
index a5dd814..fb407da 100644
--- a/include/linux/hiddev.h
+++ b/include/linux/hiddev.h
@@ -32,6 +32,17 @@
* In-kernel definitions.
*/
+struct hiddev {
+ int minor;
+ int exist;
+ int open;
+ struct mutex existancelock;
+ wait_queue_head_t wait;
+ struct hid_device *hid;
+ struct list_head list;
+ spinlock_t list_lock;
+};
+
struct hid_device;
struct hid_usage;
struct hid_field;
--
2.7.4
[PATCH v2 0/2] HID: hiddev: move hiddev's minor number and refactoring
Hi all, I found hiddev's minor number is always zero in struct hid_device. So, store the minor number asked from usb core in struct hid_device. This is my first approach. But after reviewed from Bendjamin, he suggested that it would make sense to store a minor number in struct hiddev like hidraw if it neeeded. So, I move the minor number from hid_device to hiddev and do some refactoring to access struct hiddev in hid-core Chanes in v2: - cp2112: tie in this series - hiddev: rollback struct hiddev_list exported squash this patch in a seperate patch Jaejoong Kim (2): HID: cp2112: use proper hidraw name with minor number HID: hiddev: reallocate hiddev's minor number drivers/hid/hid-core.c | 2 +- drivers/hid/hid-cp2112.c| 4 +++- drivers/hid/usbhid/hiddev.c | 15 --- include/linux/hid.h | 1 - include/linux/hiddev.h | 11 +++ 5 files changed, 19 insertions(+), 14 deletions(-) -- 2.7.4
[PATCH v2 1/2] HID: cp2112: use proper hidraw name with minor number
The cp2112 driver is working on hidraw not hiddev. So we need to use proper hidraw name with hidraw's minor number. Reviewed-by: Benjamin Tissoires Signed-off-by: Jaejoong Kim --- Changes in v2: - tie in a series --- drivers/hid/hid-cp2112.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index b22d0f8..078026f 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -1297,7 +1298,8 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) dev->adap.algo_data = dev; dev->adap.dev.parent= &hdev->dev; snprintf(dev->adap.name, sizeof(dev->adap.name), -"CP2112 SMBus Bridge on hiddev%d", hdev->minor); +"CP2112 SMBus Bridge on hidraw%d", +((struct hidraw *)hdev->hidraw)->minor); dev->hwversion = buf[2]; init_waitqueue_head(&dev->wait); -- 2.7.4
[PATCH 0/2] HID: hiddev: move hiddev's minor number and refactoring
Hi all, I found hiddev's minor number is always zero in struct hid_device. So, store the minor number asked from usb core in struct hid_device. This is my first approach. But after reviewed from Bendjamin, he suggested that it would make sense to store a minor number in struct hiddev like hidraw if it neeeded. So, I move the minor number from hid_device to hiddev and do some refactoring to access struct hiddev in hid-core Jaejoong Kim (2): HID: hiddev: move hiddev's minor number from struct hid_device to hiddev HID: hiddev: store hiddev's minor number when hiddev is connected drivers/hid/hid-core.c | 2 +- drivers/hid/usbhid/hiddev.c | 25 +++-- include/linux/hid.h | 1 - include/linux/hiddev.h | 24 4 files changed, 28 insertions(+), 24 deletions(-) -- 2.7.4
[PATCH 1/2] HID: hiddev: move hiddev's minor number from struct hid_device to hiddev
We need to store the minor number each drivers. In case of hidraw, it's
minor number stores in struct hidraw. But hiddev's minor is located in
struct hid_device.
So reallocates for hiddev's minor number.
Signed-off-by: Jaejoong Kim
---
drivers/hid/usbhid/hiddev.c | 1 +
include/linux/hid.h | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 700145b..5c2c489 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -47,6 +47,7 @@
#define HIDDEV_BUFFER_SIZE 2048
struct hiddev {
+ int minor;
int exist;
int open;
struct mutex existancelock;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 28f38e2b8..643c017 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -541,7 +541,6 @@ struct hid_device {
/* device report descriptor */
struct list_head inputs;/* The
list of inputs */
void *hiddev; /* The
hiddev structure */
void *hidraw;
- int minor; /*
Hiddev minor number */
int open; /* is
the device open by anyone? */
char name[128]; /*
Device name */
--
2.7.4
[PATCH 2/2] HID: hiddev: store hiddev's minor number when hiddev is connected
The hid-core announces kernel message which driver is loaded if there is
a hiddev, but hiddev's minor number is always zero even though it's not
zero.
So, we need to store the minor number asked from usb core and do some
refactoring work(move from hiddev.c to hiddev.h) to access hiddev in
hid-core.
Signed-off-by: Jaejoong Kim
---
drivers/hid/hid-core.c | 2 +-
drivers/hid/usbhid/hiddev.c | 26 +++---
include/linux/hiddev.h | 24
3 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index e9e87d3..1a0b910 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1695,7 +1695,7 @@ int hid_connect(struct hid_device *hdev, unsigned int
connect_mask)
len += sprintf(buf + len, "input");
if (hdev->claimed & HID_CLAIMED_HIDDEV)
len += sprintf(buf + len, "%shiddev%d", len ? "," : "",
- hdev->minor);
+ ((struct hiddev *)hdev->hiddev)->minor);
if (hdev->claimed & HID_CLAIMED_HIDRAW)
len += sprintf(buf + len, "%shidraw%d", len ? "," : "",
((struct hidraw *)hdev->hidraw)->minor);
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 5c2c489..ef83d68 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -44,29 +44,6 @@
#define HIDDEV_MINOR_BASE 96
#define HIDDEV_MINORS 16
#endif
-#define HIDDEV_BUFFER_SIZE 2048
-
-struct hiddev {
- int minor;
- int exist;
- int open;
- struct mutex existancelock;
- wait_queue_head_t wait;
- struct hid_device *hid;
- struct list_head list;
- spinlock_t list_lock;
-};
-
-struct hiddev_list {
- struct hiddev_usage_ref buffer[HIDDEV_BUFFER_SIZE];
- int head;
- int tail;
- unsigned flags;
- struct fasync_struct *fasync;
- struct hiddev *hiddev;
- struct list_head node;
- struct mutex thread_lock;
-};
/*
* Find a report, given the report's type and ID. The ID can be specified
@@ -911,6 +888,9 @@ int hiddev_connect(struct hid_device *hid, unsigned int
force)
kfree(hiddev);
return -1;
}
+
+ hiddev->minor = usbhid->intf->minor;
+
return 0;
}
diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
index a5dd814..ff3701b 100644
--- a/include/linux/hiddev.h
+++ b/include/linux/hiddev.h
@@ -32,6 +32,30 @@
* In-kernel definitions.
*/
+#define HIDDEV_BUFFER_SIZE 2048
+
+struct hiddev {
+ int minor;
+ int exist;
+ int open;
+ struct mutex existancelock;
+ wait_queue_head_t wait;
+ struct hid_device *hid;
+ struct list_head list;
+ spinlock_t list_lock;
+};
+
+struct hiddev_list {
+ struct hiddev_usage_ref buffer[HIDDEV_BUFFER_SIZE];
+ int head;
+ int tail;
+ unsigned flags;
+ struct fasync_struct *fasync;
+ struct hiddev *hiddev;
+ struct list_head node;
+ struct mutex thread_lock;
+};
+
struct hid_device;
struct hid_usage;
struct hid_field;
--
2.7.4
