We (ChromeOS) got a syzkaller report of a KASAN use after free read in
hidinput_find_key(). The callstack is from evdev_ioctl() calling
hidinput_setkeycode():
__asan_report_load4_noabort+0x44/0x50
hidinput_find_key+0x25c/0x340
hidinput_locate_usage+0x31c/0x400
hidinput_setkeycode+0x70/0x460
input_set_keycode+0xd4/0x3f8
evdev_do_ioctl+0x2508/0x6678
evdev_ioctl_handler+0x12c/0x180
evdev_ioctl+0x40/0x54
The memory being read was allocated during hammer_probe() by
hid_open_report():
Allocated by task 19025:
kasan_save_stack+0x38/0x68
__kasan_kmalloc+0x90/0xac
__kmalloc+0x27c/0x45c
hid_add_field+0x4b0/0x125c
hid_parser_main+0x214/0x994
hid_open_report+0x388/0x7a8
hammer_probe+0x80/0x698 [hid_google_hammer]
and the memory was freed by hid_close_report() called from
hid_destroy_device().
Freed by task 19025:
kasan_save_stack+0x38/0x68
kasan_set_track+0x28/0x3c
kasan_set_free_info+0x28/0x4c
____kasan_slab_free+0x110/0x164
__kasan_slab_free+0x18/0x28
kfree+0x208/0x950
hid_close_report+0xd0/0x29c
hid_device_remove+0x104/0x198
device_release_driver_internal+0x204/0x400
device_release_driver+0x30/0x40
bus_remove_device+0x2a0/0x390
device_del+0x49c/0x858
hid_destroy_device+0x78/0x11c
usbhid_disconnect+0xb4/0x100
usb_unbind_interface+0x178/0x6f4
device_release_driver_internal+0x240/0x400
device_release_driver+0x30/0x40
bus_remove_device+0x2a0/0x390
The memory that's being read by the ioctl is an HID report that's been
freed when the HID device is destroyed because the usb interface is
unbound. In hid_device_remove() we assume that the hid report can be
closed with hid_close_report() after the hid_driver is unbound, which is
generally safe because the driver should have stopped the hardware with
hid_hw_stop() when it was unbound. In fact, hid_device_remove() falls
back to calling hid_hw_stop() directly if the hid driver doesn't have a
remove() function, so the assumption is that hid_hw_stop() has been
called once the hid_driver::remove() function returns. hid_hw_stop()
will eventually call hidinput_disconnect() which will unregister the
hidinput device; ensuring that userspace can't call ioctls on the
hidinput device when hid_hw_stop() returns.
Unfortunately, the hid google hammer driver hand rolls a devm function
to call hid_hw_stop() when the driver is unbound and implements an
hid_driver::remove() function. The driver core doesn't call the devm
release functions until _after_ the bus unbinds the driver, so the order
of operations is like this:
__device_release_driver()
...
device_remove(dev)
hid_device_remove(hdev)
hdrv->remove(hdev);
hid_close_report(hdev) <---- Frees the report
device_unbind_cleanup(dev)
devres_release_all(dev)
...
hid_hw_stop(hdev) <--- Removes the hid_input device
We want the order of operations to be hid_hw_stop() and then
hid_close_report() so that the report can be freed without the hid_input
device hanging around attempting to deref the report. Remove the hand
rolled devm function and call hid_hw_stop() from the hammer_remove()
function to fix the ordering.
Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
Fixes: d950db3f80a8 ("HID: google: switch to devm when registering keyboard backlight LED")
Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
---
drivers/hid/hid-google-hammer.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 7ae5f27df54d..e7f7c3c68747 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -495,11 +495,6 @@ static void hammer_get_folded_state(struct hid_device *hdev)
kfree(buf);
}
-static void hammer_stop(void *hdev)
-{
- hid_hw_stop(hdev);
-}
-
static int hammer_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
@@ -520,10 +515,6 @@ static int hammer_probe(struct hid_device *hdev,
if (error)
return error;
- error = devm_add_action(&hdev->dev, hammer_stop, hdev);
- if (error)
- return error;
-
/*
* We always want to poll for, and handle tablet mode events from
* devices that have folded usage, even when nobody has opened the input
@@ -533,8 +524,10 @@ static int hammer_probe(struct hid_device *hdev,
if (hammer_has_folded_event(hdev)) {
hdev->quirks |= HID_QUIRK_ALWAYS_POLL;
error = hid_hw_open(hdev);
- if (error)
+ if (error) {
+ hid_hw_stop(hdev);
return error;
+ }
hammer_get_folded_state(hdev);
}
@@ -576,7 +569,8 @@ static void hammer_remove(struct hid_device *hdev)
spin_unlock_irqrestore(&cbas_ec_lock, flags);
}
- /* Unregistering LEDs and stopping the hardware is done via devm */
+ /* Unregistering LEDs is done via devm */
+ hid_hw_stop(hdev);
}
static const struct hid_device_id hammer_devices[] = {
base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
--
https://chromeos.dev
