Linux Input/HID development
 help / color / mirror / Atom feed
* Re: [PATCH 1/6] HID: roccat-koneplus: reject short button reports
From: sashiko-bot @ 2026-06-28 17:01 UTC (permalink / raw)
  To: Yousef Alhouseen; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260628164611.17467-1-alhouseenyousef@gmail.com>

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Use-After-Free during device teardown due to incorrect removal ordering.
- [High] Incomplete Fix: Out-of-bounds read via short USB reports in the savu driver.
- [High] Memory leak of `struct roccat_reader` and `struct roccat_device` on hot-unplug.
- [High] Sleeping in atomic context due to mutex_lock in the raw_event path.
- [High] Lockless Access and Data Race on the `devices` array causing NULL pointer dereference or UAF.
- [High] Kernel information leak via sysfs attributes due to uninitialized memory copying and ignored errors.
- [Low] Unsynchronized access to `actual_profile` between process and interrupt context.
--

commit d5a3862120a746721ced4fff1b161538d9d1d57c
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: roccat-koneplus: reject short button reports

The Koneplus raw-event path casts button reports to an eight-byte
structure before updating profile state and forwarding an event. A
malformed USB device can identify a shorter report as a button report
and trigger out-of-bounds reads.

Require the complete button report before either consumer sees it.

Fixes: 47dbdbffe15b ("HID: roccat: Add support for Roccat Kone[+] v2")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>

> diff --git a/drivers/hid/hid-roccat-koneplus.c b/drivers/hid/hid-roccat-koneplus.c
> index f80a60539a967..e0f35251e81a9 100644
> --- a/drivers/hid/hid-roccat-koneplus.c
> +++ b/drivers/hid/hid-roccat-koneplus.c
> @@ -523,6 +523,10 @@ static int koneplus_raw_event(struct hid_device *hdev,
>  	if (koneplus == NULL)
>  		return 0;
>  
> +	if (data[0] == KONEPLUS_MOUSE_REPORT_NUMBER_BUTTON &&
> +	    size < sizeof(struct koneplus_mouse_report_button))
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but does the savu driver also need a similar
check?

It looks like the savu driver lacks length validation. savu_raw_event()
forwards reports to savu_report_to_chrdev(), which casts the buffer:

drivers/hid/hid-roccat-savu.c:savu_report_to_chrdev() {
    ...
    roccat_report.type = special_report->type;
    roccat_report.data[0] = special_report->data[0];
    roccat_report.data[1] = special_report->data[1];
    ...
}

If a malicious USB device sends a 1-byte report to the savu driver, could
these accesses read out-of-bounds of the heap buffer and leak information
to userspace via the character device?

>  	koneplus_keep_values_up_to_date(koneplus, data);

[Severity: Low]
This isn't a bug introduced by this patch, but is it safe to update the
profile state here without synchronization?

koneplus->actual_profile is updated from sysfs in process context under
koneplus_lock, but it is also updated here in interrupt context via
koneplus_keep_values_up_to_date() without acquiring any locks:

drivers/hid/hid-roccat-koneplus.c:koneplus_keep_values_up_to_date() {
    ...
    koneplus_profile_activated(koneplus, button_report->data1 - 1);
    ...
}

Could this data race cause stale profile values to be read if sysfs
attributes are accessed concurrently with profile-changing USB events?

>  
>  	if (koneplus->roccat_claimed)

[Severity: High]
This is a pre-existing issue, but can this path sleep in atomic context?

koneplus_raw_event() runs in atomic interrupt context from the USB URB
completion handler. If roccat_claimed is true, it calls
koneplus_report_to_chrdev(), which eventually calls roccat_report_event():

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    mutex_lock(&device->readers_lock);
    mutex_lock(&device->cbuf_lock);
    ...
}

Would acquiring these mutexes inside the raw_event path trigger a
scheduling-while-atomic bug upon receiving valid input events?

[Severity: High]
This is another pre-existing issue, but does roccat_report_event() have a
data race with device removal?

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    device = devices[minor];
    
    new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
    ...
}

If roccat_disconnect() runs concurrently, takes devices_lock, and sets
devices[minor] = NULL, could roccat_report_event() fetch a NULL or freed
device pointer locklessly, leading to a crash or use-after-free when
accessing device->report_size?

I also noticed a few other pre-existing issues in the ROCCAT subsystem
that might be worth addressing:

[Severity: Critical]
This is a pre-existing issue, but could there be a use-after-free during
device teardown?

In koneplus_remove(), the driver state is freed before the hardware is
stopped:

drivers/hid/hid-roccat-koneplus.c:koneplus_remove() {
    ...
    koneplus_remove_specials(hdev);
    hid_hw_stop(hdev);
    ...
}

If a USB event arrives exactly during this window before hid_hw_stop()
halts the in-flight USB URBs, koneplus_raw_event() will fetch the
freed driver data:

drivers/hid/hid-roccat-koneplus.c:koneplus_raw_event() {
    struct koneplus_device *koneplus = hid_get_drvdata(hdev);
    ...
    koneplus_keep_values_up_to_date(koneplus, data);
    ...
}

Would this dereference the freed koneplus pointer?

[Severity: High]
This is a pre-existing issue, but does hot-unplugging the device while it
is held open by userspace cause a memory leak?

When a ROCCAT device is unplugged, roccat_disconnect() clears the array slot:

drivers/hid/hid-roccat.c:roccat_disconnect() {
    ...
    devices[minor] = NULL;
    ...
}

If a user-space process holds the char device open, the memory isn't freed
immediately. When the process later closes the file descriptor,
roccat_release() checks the array:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
    device = devices[minor];
    if (!device) {
        mutex_unlock(&devices_lock);
        pr_emerg("roccat device with minor %d doesn't exist\n", minor);
        return -ENODEV;
    }
    ...
}

Since the device was set to NULL during disconnect, it returns early and
bypasses freeing the reader and device structures. Are these structures
leaked permanently?

[Severity: High]
This is a pre-existing issue, but can sysfs attributes leak uninitialized
heap memory to userspace?

In roccat_common2_receive(), a buffer is allocated via kmalloc() without
GFP_ZERO. If usb_control_msg() fails, it still executes memcpy():

drivers/hid/hid-roccat-common.c:roccat_common2_receive() {
    ...
    len = usb_control_msg(...);
    
    memcpy(data, buf, size);
    ...
}

Sysfs show handlers ignore the return value and emit the potentially
uninitialized data:

drivers/hid/hid-roccat-koneplus.c:koneplus_sysfs_show_firmware_version() {
    ...
    roccat_common2_receive(usb_dev, KONEPLUS_COMMAND_INFO,
                    &info, KONEPLUS_SIZE_INFO);
    ...
    return sysfs_emit(buf, "%d\n", info.firmware_version);
}

If the USB transfer fails, does this expose uninitialized memory to any
local user able to read the sysfs files?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628164611.17467-1-alhouseenyousef@gmail.com?part=1

^ permalink raw reply

* Re: [PATCH 6/6] HID: roccat-ryos: reject short special reports
From: sashiko-bot @ 2026-06-28 17:01 UTC (permalink / raw)
  To: Yousef Alhouseen; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260628164611.17467-6-alhouseenyousef@gmail.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `roccat_report_event()` function acquires sleepable mutexes while running in an atomic context (USB URB completion handler), which leads to a "scheduling while atomic" BUG.
--

commit de88f5cf3ef84f642bf5e1815c5ed0d7ee05a061
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: roccat-ryos: reject short special reports

This commit fixes an out-of-bounds read in ryos_raw_event() by ensuring
that only complete special reports are forwarded to the character-device
layer. It drops reports that are shorter than the expected report size.

> diff --git a/drivers/hid/hid-roccat-ryos.c b/drivers/hid/hid-roccat-ryos.c
> index db83f42457da5..5087defc7c373 100644
> --- a/drivers/hid/hid-roccat-ryos.c
> +++ b/drivers/hid/hid-roccat-ryos.c
> @@ -189,7 +189,8 @@ static int ryos_raw_event(struct hid_device *hdev,
>  			!= RYOS_USB_INTERFACE_PROTOCOL)
>  		return 0;
>  
> -	if (data[0] != RYOS_REPORT_NUMBER_SPECIAL)
> +	if (data[0] != RYOS_REPORT_NUMBER_SPECIAL ||
> +	    size < sizeof(struct ryos_report_special))
>  		return 0;
>  
>  	if (ryos != NULL && ryos->roccat_claimed)
>  		roccat_report_event(ryos->chrdev_minor, data);

[Severity: High]
This is a pre-existing issue, but is it safe to call roccat_report_event()
from this context?

The ryos_raw_event() callback executes in an atomic context (softirq)
during the USB URB completion handler:

hid_irq_in()
  hid_safe_input_report()
    __hid_input_report()
      hdrv->raw_event() -> ryos_raw_event()

However, looking at drivers/hid/hid-roccat.c, roccat_report_event() acquires
sleepable mutexes:

roccat_report_event()
{
        ...
        mutex_lock(&device->readers_lock);
        mutex_lock(&device->cbuf_lock);
        ...
}

Will this sequence trigger a scheduling while atomic bug when a valid
special report is received and forwarded?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628164611.17467-1-alhouseenyousef@gmail.com?part=6

^ permalink raw reply

* Re: [PATCH] HID: hiddev: keep state alive through disconnect unlock
From: Hillf Danton @ 2026-06-28 22:26 UTC (permalink / raw)
  To: Yousef Alhouseen
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-usb,
	linux-kernel, syzbot+563191a4939ddbfe73d4
In-Reply-To: <20260628093245.42065-1-alhouseenyousef@gmail.com>

On Sun, 28 Jun 2026 11:32:45 +0200 Yousef Alhouseen wrote:
> hiddev_disconnect() drops existancelock before freeing the hiddev state,
> but a waiting final file release can run as soon as the mutex becomes
> available. On PREEMPT_RT, that waiter may free hiddev while the disconnect
> thread is still executing the mutex slow-unlock path, causing a
> use-after-free in the mutex implementation.
> 
The root cause is not specified as clearly as expected.

	hiddev_disconnect()	hiddev_release()
	---			---
	mutex_lock(&hiddev->existancelock);
	hiddev->exist = 0;

	if (hiddev->open) {
		hid_hw_close(hiddev->hid);
		wake_up_interruptible(&hiddev->wait);
		mutex_unlock(&hiddev->existancelock);
		__mutex_unlock_slowpath()
		raw_spin_lock_irqsave()   //UAF

				mutex_lock(&list->hiddev->existancelock);
				if (!--list->hiddev->open) {
					if (list->hiddev->exist) {
						hid_hw_close(list->hiddev->hid);
						hid_hw_power(list->hiddev->hid, PM_HINT_NORMAL);
					} else {
						mutex_unlock(&list->hiddev->existancelock);
						kfree(list->hiddev); // free mem
						vfree(list);
						return 0;
					}
				}
				mutex_unlock(&list->hiddev->existancelock);

Given the syzbot report, uaf occured upon acquiring the raw spinlock in
__mutex_unlock_slowpath(), but no mutex waiter could be waken up without
the raw spinlock held, thus the report sounds false positive.

> Give the connection and each open file an explicit reference. Drop each
> reference only after its existancelock critical section has completed, so
> the state cannot be freed from the other unlock path.
> 
> Fixes: 079034073faf ("HID: hiddev cleanup -- handle all error conditions properly")
> Reported-by: syzbot+563191a4939ddbfe73d4@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4
> Cc: stable@vger.kernel.org
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---
>  drivers/hid/usbhid/hiddev.c | 37 +++++++++++++++++++------------------
>  include/linux/hiddev.h      |  2 ++
>  2 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 6378801b22c6..21396481995b 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -46,6 +46,12 @@ struct hiddev_list {
>  	struct mutex thread_lock;
>  };
>  
> +static void hiddev_put(struct hiddev *hiddev)
> +{
> +	if (refcount_dec_and_test(&hiddev->refcount))
> +		kfree(hiddev);
> +}
> +
>  /*
>   * Find a report, given the report's type and ID.  The ID can be specified
>   * indirectly by REPORT_ID_FIRST (which returns the first report of the given
> @@ -216,26 +222,21 @@ static int hiddev_fasync(int fd, struct file *file, int on)
>  static int hiddev_release(struct inode * inode, struct file * file)
>  {
>  	struct hiddev_list *list = file->private_data;
> +	struct hiddev *hiddev = list->hiddev;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&list->hiddev->list_lock, flags);
> +	spin_lock_irqsave(&hiddev->list_lock, flags);
>  	list_del(&list->node);
> -	spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
> +	spin_unlock_irqrestore(&hiddev->list_lock, flags);
>  
> -	mutex_lock(&list->hiddev->existancelock);
> -	if (!--list->hiddev->open) {
> -		if (list->hiddev->exist) {
> -			hid_hw_close(list->hiddev->hid);
> -			hid_hw_power(list->hiddev->hid, PM_HINT_NORMAL);
> -		} else {
> -			mutex_unlock(&list->hiddev->existancelock);
> -			kfree(list->hiddev);
> -			vfree(list);
> -			return 0;
> -		}
> +	mutex_lock(&hiddev->existancelock);
> +	if (!--hiddev->open && hiddev->exist) {
> +		hid_hw_close(hiddev->hid);
> +		hid_hw_power(hiddev->hid, PM_HINT_NORMAL);
>  	}
>  
> -	mutex_unlock(&list->hiddev->existancelock);
> +	mutex_unlock(&hiddev->existancelock);
> +	hiddev_put(hiddev);
>  	vfree(list);
>  
>  	return 0;
> @@ -270,6 +271,7 @@ static int __hiddev_open(struct hiddev *hiddev, struct file *file)
>  	spin_unlock_irq(&hiddev->list_lock);
>  
>  	file->private_data = list;
> +	refcount_inc(&hiddev->refcount);
>  
>  	return 0;
>  
> @@ -897,6 +899,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>  	INIT_LIST_HEAD(&hiddev->list);
>  	spin_lock_init(&hiddev->list_lock);
>  	mutex_init(&hiddev->existancelock);
> +	refcount_set(&hiddev->refcount, 1);
>  	hid->hiddev = hiddev;
>  	hiddev->hid = hid;
>  	hiddev->exist = 1;
> @@ -937,9 +940,7 @@ void hiddev_disconnect(struct hid_device *hid)
>  	if (hiddev->open) {
>  		hid_hw_close(hiddev->hid);
>  		wake_up_interruptible(&hiddev->wait);
> -		mutex_unlock(&hiddev->existancelock);
> -	} else {
> -		mutex_unlock(&hiddev->existancelock);
> -		kfree(hiddev);
>  	}
> +	mutex_unlock(&hiddev->existancelock);
> +	hiddev_put(hiddev);
>  }
> diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
> index 2164c03d2c72..8e9f8a33e359 100644
> --- a/include/linux/hiddev.h
> +++ b/include/linux/hiddev.h
> @@ -13,6 +13,7 @@
>  #ifndef _HIDDEV_H
>  #define _HIDDEV_H
>  
> +#include <linux/refcount.h>
>  #include <uapi/linux/hiddev.h>
>  
>  
> @@ -24,6 +25,7 @@ struct hiddev {
>  	int minor;
>  	int exist;
>  	int open;
> +	refcount_t refcount;
>  	struct mutex existancelock;
>  	wait_queue_head_t wait;
>  	struct hid_device *hid;
> -- 
> 2.54.0

^ permalink raw reply

* [PATCH] Input: maplemouse - fix NULL pointer dereference in open()
From: Florian Fuchs @ 2026-06-28 23:07 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input
  Cc: linux-sh, Guenter Roeck, linux-kernel, Florian Fuchs

Commit 555c765b0cc2 ("Input: mouse - drop unnecessary calls to
input_set_drvdata") dropped the input_set_drvdata() call in probe
because the data appeared to be unused. However, dc_mouse_open() and
dc_mouse_close() were using maple_get_drvdata(to_maple_dev(&dev->dev)).
This actually retrieves driver data from the input device's embedded
struct device. After input_set_drvdata() was removed, that lookup started
returning NULL and opening the input device dereferences mse->mdev.

Restore input_set_drvdata() and convert open() and close() to use
input_get_drvdata() so the dependency is no longer hidden.

Fixes: 555c765b0cc2 ("Input: mouse - drop unnecessary calls to input_set_drvdata")
Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
---
This fix was tested on the target platform. The following is the error I
get, when using the unpatched kernel:

BUG: unable to handle kernel NULL pointer dereference at 00000004
PC: [<8c26eec4>] dc_mouse_open+0xc/0x28
pgd = f700ee57
[00000004] *pgd=00000000
Oops: 0000 [#1]

CPU: 0 UID: 0 PID: 45 Comm: Xfbdev Not tainted 7.1.1 #84 PREEMPT
PC is at dc_mouse_open+0xc/0x28
PR is at input_open_device+0x7c/0xe0
PC  : 8c26eec4 SP  : 8c7bbd9c SR  : 40008100 TEA : 00000004
R0  : 8c26eeb8 R1  : 00000000 R2  : 00000001 R3  : 00000000
R4  : 8c6b0dc0 R5  : 8c26efa8 R6  : 8c7b64c0 R7  : 00000200
R8  : 00000000 R9  : 8c6b0d70 R10 : 8c6b0c00 R11 : 8c6ce604
R12 : 8c390a64 R13 : 8c6b0d3c R14 : 8c0e9ba0
MACH: 00000006 MACL: 8686868d GBR : 29609ff4 PR  : 8c265fc8

Call trace:
 [<8c265fc8>] input_open_device+0x7c/0xe0
 [<8c26b2d0>] mousedev_open_device+0x38/0x68
 [<8c26b77c>] mousedev_open+0xa4/0x110
 [<8c0e9cc6>] chrdev_open+0x112/0x15c
 [<8c0e2e42>] do_dentry_open+0x27e/0x2fc
 [<8c0e9bb4>] chrdev_open+0x0/0x15c
 [<8c0f32d2>] path_openat+0x1d2/0x7cc
 [<8c0f3956>] do_file_open+0x8a/0xf0
 [<8c0f3100>] path_openat+0x0/0x7cc
 [<8c1efeac>] strncpy_from_user+0x64/0xe4
 [<8c0ffc7e>] alloc_fd+0x106/0x124
 [<8c0e41ed>] sys_openat2+0xb9/0xbc
 [<8c0e3fc6>] do_sys_openat2+0x76/0xd4
 [<8c0e40ee>] do_sys_open+0x2a/0x54
 [<8c00e25a>] syscall_call+0x18/0x1e
 [<8c0e4118>] sys_open+0x0/0x10

 drivers/input/mouse/maplemouse.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/maplemouse.c b/drivers/input/mouse/maplemouse.c
index c99f7e234219..c41182766538 100644
--- a/drivers/input/mouse/maplemouse.c
+++ b/drivers/input/mouse/maplemouse.c
@@ -48,7 +48,7 @@ static void dc_mouse_callback(struct mapleq *mq)
 
 static int dc_mouse_open(struct input_dev *dev)
 {
-	struct dc_mouse *mse = maple_get_drvdata(to_maple_dev(&dev->dev));
+	struct dc_mouse *mse = input_get_drvdata(dev);
 
 	maple_getcond_callback(mse->mdev, dc_mouse_callback, HZ/50,
 		MAPLE_FUNC_MOUSE);
@@ -58,7 +58,7 @@ static int dc_mouse_open(struct input_dev *dev)
 
 static void dc_mouse_close(struct input_dev *dev)
 {
-	struct dc_mouse *mse = maple_get_drvdata(to_maple_dev(&dev->dev));
+	struct dc_mouse *mse = input_get_drvdata(dev);
 
 	maple_getcond_callback(mse->mdev, dc_mouse_callback, 0,
 		MAPLE_FUNC_MOUSE);
@@ -88,6 +88,7 @@ static int probe_maple_mouse(struct device *dev)
 	mse->dev = input_dev;
 	mse->mdev = mdev;
 
+	input_set_drvdata(input_dev, mse);
 	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
 	input_dev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
 		BIT_MASK(BTN_RIGHT) | BIT_MASK(BTN_MIDDLE);
-- 
2.43.0


^ permalink raw reply related

* Re: [PATCH] Input: maplemouse - fix NULL pointer dereference in open()
From: sashiko-bot @ 2026-06-28 23:27 UTC (permalink / raw)
  To: Florian Fuchs; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260628230715.2982552-1-fuchsfl@gmail.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Initialization of maple bus driver data occurs after exposing the input device to userspace, leading to a potential NULL pointer dereference.
- [High] Teardown asymmetry causes a race condition where a background callback is inadvertently re-armed during device removal, leading to UAF or NULL pointer dereference.
--

commit 5babe5754528798c9e3476330c1a7834fe83388a
Author: Florian Fuchs <fuchsfl@gmail.com>

Input: maplemouse - fix NULL pointer dereference in open()

Commit 555c765b0cc2 dropped the input_set_drvdata() call in probe, which
caused a NULL pointer dereference in open() because maple_get_drvdata()
was still being used. This patch restores input_set_drvdata() and
converts open() and close() to use input_get_drvdata().

> diff --git a/drivers/input/mouse/maplemouse.c b/drivers/input/mouse/maplemouse.c
> index c99f7e2342191..c41182766538a 100644
> --- a/drivers/input/mouse/maplemouse.c
> +++ b/drivers/input/mouse/maplemouse.c

[ ... ]

> @@ -58,7 +58,7 @@ static int dc_mouse_open(struct input_dev *dev)
>  
>  static void dc_mouse_close(struct input_dev *dev)
>  {
> -	struct dc_mouse *mse = maple_get_drvdata(to_maple_dev(&dev->dev));
> +	struct dc_mouse *mse = input_get_drvdata(dev);
>  
>  	maple_getcond_callback(mse->mdev, dc_mouse_callback, 0,
>  		MAPLE_FUNC_MOUSE);
>  }

[Severity: High]
This is a pre-existing issue, but does calling maple_getcond_callback() here
create a race condition during device removal?

During removal, remove_maple_mouse() clears the callback before unregistering
the device:

drivers/input/mouse/maplemouse.c:remove_maple_mouse() {
	...
	mdev->callback = NULL;
	input_unregister_device(mse->dev);
	...
}

If the device is open, input_unregister_device() synchronously calls
dc_mouse_close(). This unconditionally calls maple_getcond_callback(), which
re-arms mdev->callback back to dc_mouse_callback.

After input_unregister_device() returns, remove_maple_mouse() clears the
driver data and frees mse. If a delayed maple bus packet arrives after this,
dc_mouse_callback() will execute and attempt to use a freed mse pointer or a
NULL driver data pointer, causing a kernel panic.

Would it be safer to avoid re-arming the callback if the device is being
removed?

[ ... ]

> @@ -88,6 +88,7 @@ static int probe_maple_mouse(struct device *dev)
>  	mse->dev = input_dev;
>  	mse->mdev = mdev;
>  
> +	input_set_drvdata(input_dev, mse);
>  	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
>  	input_dev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
>  		BIT_MASK(BTN_RIGHT) | BIT_MASK(BTN_MIDDLE);

[Severity: High]
This is a pre-existing issue, but is it possible for a callback to fire
before driver data is fully initialized?

Later in probe_maple_mouse(), the device is registered:

drivers/input/mouse/maplemouse.c:probe_maple_mouse() {
	...
	error = input_register_device(input_dev);
	if (error)
		goto fail_register;

	mdev->driver = mdrv;
	maple_set_drvdata(mdev, mse);
	...
}

If userspace immediately opens the device upon registration, dc_mouse_open()
will schedule dc_mouse_callback(). If the callback executes before
maple_set_drvdata() completes, maple_get_drvdata(mapledev) inside the
callback will return NULL, leading to a NULL pointer dereference when
accessing mse->dev.

Could the maple_set_drvdata() call be moved before input_register_device()
to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628230715.2982552-1-fuchsfl@gmail.com?part=1

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox