| Message ID | 20191122202559.GA71021@dtor-ws (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Benjamin Tissoires |
| Headers | show |
| Series | HID: hid-input: clear unmapped usages | expand |
Hi Dmitry, On Fri, Nov 22, 2019 at 9:26 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > We should not be leaving half-mapped usages with potentially invalid > keycodes, as that may confuse hidinput_find_key() when the key is > located by index, which may end up feeding way too large keycode into > the VT keyboard handler and cause OOB write there: > > BUG: KASAN: global-out-of-bounds in clear_bit include/asm-generic/bitops-instrumented.h:56 [inline] > BUG: KASAN: global-out-of-bounds in kbd_keycode drivers/tty/vt/keyboard.c:1411 [inline] > BUG: KASAN: global-out-of-bounds in kbd_event+0xe6b/0x3790 drivers/tty/vt/keyboard.c:1495 > Write of size 8 at addr ffffffff89a1b2d8 by task syz-executor108/1722 > ... > kbd_keycode drivers/tty/vt/keyboard.c:1411 [inline] > kbd_event+0xe6b/0x3790 drivers/tty/vt/keyboard.c:1495 > input_to_handler+0x3b6/0x4c0 drivers/input/input.c:118 > input_pass_values.part.0+0x2e3/0x720 drivers/input/input.c:145 > input_pass_values drivers/input/input.c:949 [inline] > input_set_keycode+0x290/0x320 drivers/input/input.c:954 > evdev_handle_set_keycode_v2+0xc4/0x120 drivers/input/evdev.c:882 > evdev_do_ioctl drivers/input/evdev.c:1150 [inline] > > In this case we were dealing with a fuzzed HID device that declared over > 12K buttons: > > https://syzkaller.appspot.com/bug?extid=19340dff067c2d3835c0 > > Reported-by: syzbot+19340dff067c2d3835c0@syzkaller.appspotmail.com > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > I'll be putting a guard into drivers/tty/vt/keyboard.c as well. > Please consider for stable. > > drivers/hid/hid-input.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 63855f275a38..3957d1c4d967 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -1215,9 +1215,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > set_bit(MSC_SCAN, input->mscbit); > } > > -ignore: > return; > > +ignore: > + usage->type = 0; > + usage->code = 0; Unfortunately, this breaks the buttons on the MS precision Touchpads. The hid-tools test suite found that for me :) The problem is: hid-multitouch.c|mt_touch_input_mapping() lines 840-857 -> we assign the button mapping correctly, but there is a catch for the first button: hid-multitouch.c|mt_process_mt_event() lines 1123-1126 -> we check for the usage type and code to know which button we have We are entering the ignore case because: hid-input.c|hidinput_configure_usage() lines 582-589 -> hid-multitouch.c|mt_touch_input_mapping() lines 840-857 -> return 1 -> goto mapped hid-input.c|hidinput_configure_usage() lines 1134-1137 -> hid-multitouch.c|mt_input_mapped() lines 1360-1363 -> return -1 (we are in the mt collection) -> goto ignore We would need to change the input_mapped() function to either ignore or exit hidinput_configure_usage() based on the return value, or we would need to store the original usages in hid-multitouch. Actually, to fix this without breaking current drivers, we should just change the `goto ignore` into a `return` after checking the value of .input_mapped() Cheers, Benjamin > } > > static void hidinput_handle_scroll(struct hid_usage *usage, > -- > 2.24.0.432.g9d3f5f5b63-goog > > > -- > Dmitry >
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 63855f275a38..3957d1c4d967 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1215,9 +1215,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel set_bit(MSC_SCAN, input->mscbit); } -ignore: return; +ignore: + usage->type = 0; + usage->code = 0; } static void hidinput_handle_scroll(struct hid_usage *usage,
We should not be leaving half-mapped usages with potentially invalid keycodes, as that may confuse hidinput_find_key() when the key is located by index, which may end up feeding way too large keycode into the VT keyboard handler and cause OOB write there: BUG: KASAN: global-out-of-bounds in clear_bit include/asm-generic/bitops-instrumented.h:56 [inline] BUG: KASAN: global-out-of-bounds in kbd_keycode drivers/tty/vt/keyboard.c:1411 [inline] BUG: KASAN: global-out-of-bounds in kbd_event+0xe6b/0x3790 drivers/tty/vt/keyboard.c:1495 Write of size 8 at addr ffffffff89a1b2d8 by task syz-executor108/1722 ... kbd_keycode drivers/tty/vt/keyboard.c:1411 [inline] kbd_event+0xe6b/0x3790 drivers/tty/vt/keyboard.c:1495 input_to_handler+0x3b6/0x4c0 drivers/input/input.c:118 input_pass_values.part.0+0x2e3/0x720 drivers/input/input.c:145 input_pass_values drivers/input/input.c:949 [inline] input_set_keycode+0x290/0x320 drivers/input/input.c:954 evdev_handle_set_keycode_v2+0xc4/0x120 drivers/input/evdev.c:882 evdev_do_ioctl drivers/input/evdev.c:1150 [inline] In this case we were dealing with a fuzzed HID device that declared over 12K buttons: https://syzkaller.appspot.com/bug?extid=19340dff067c2d3835c0 Reported-by: syzbot+19340dff067c2d3835c0@syzkaller.appspotmail.com Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- I'll be putting a guard into drivers/tty/vt/keyboard.c as well. Please consider for stable. drivers/hid/hid-input.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)