Re: [PATCH v2] HID: appletb-kbd: add option to switch default layer on double pressing fn key

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Dynamic mutation of a read-only global module parameter from a per-device atomic event handler without synchronization.
- [Medium] Incorrect double-press detection during early system uptime on 32-bit systems due to jiffies wrap-around with a 0-initialized timestamp.
- [Low] Intervening non-Fn key presses do not reset the double-press detection timer.

Pre-existing issues:
- [High] Calling sleeping functions (`hid_hw_power` and `hid_hw_request`) from an atomic context causes a kernel panic.
- [High] A race condition during `probe()` allows incoming HID events to trigger a NULL pointer dereference.
--

commit 9461d5707b93702971195b71e23ddea06392c720
Author: Aditya Garg <gargaditya08@proton.me>

HID: appletb-kbd: add option to switch default layer on double pressing fn key

This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter.

> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..34bfc595949dc 100644
> --- a/drivers/hid/hid-appletb-kbd.c
> +++ b/drivers/hid/hid-appletb-kbd.c

[ ... ]

> @@ -243,6 +250,18 @@ static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *fiel
>  	return kbd->current_mode == APPLETB_KBD_MODE_OFF;
>  }
>  

[Severity: High]
This is a pre-existing issue, but does a race condition during probe() allow
incoming HID events to trigger a NULL pointer dereference?

If hid_hw_start() and hid_hw_open() are called before hid_set_drvdata() is
executed, an incoming keyboard event might invoke appletb_kbd_hid_event()
before probe() finishes. This would call hid_get_drvdata(hdev) returning
NULL, which is then passed to reset_inactivity_timer() and unconditionally
dereferenced.

> +static u8 appletb_switch_mode(u8 mode)
> +{

[ ... ]

> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
>  
>  	reset_inactivity_timer(kbd);
>  
> -	if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle &&
> -		(kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> -		 kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> +	if (type == EV_KEY && code == KEY_FN &&
> +	    (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> +	     kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> +

[Severity: Low]
Is it possible that intervening non-Fn key presses should reset the
double-press detection timer?

If a user presses Fn, types another key, and then presses Fn again within
the delay timeout, the driver might erroneously interpret this as a
double-press and switch the layer.

>  		if (value == 1) {
> -			kbd->saved_mode = kbd->current_mode;
> -			appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
> -						? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
> +			if (appletb_tb_double_press_switch_layers > 0) {
> +				unsigned long now = jiffies;
> +
> +				if (time_before(now, kbd->last_fn_press +
> +					msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {

[Severity: Medium]
Could this double-press detection trigger incorrectly during early system
uptime on 32-bit systems?

Since last_fn_press is zero-initialized and jiffies starts at a large
negative value, time_before() might evaluate to true for the first 5
minutes of uptime.

This would cause any single press of the Fn key to erroneously trigger a
double-press layer switch.

> +
> +					appletb_tb_def_mode =
> +						appletb_switch_mode(
> +							appletb_tb_def_mode);

[Severity: Medium]
Is it safe to dynamically mutate the appletb_tb_def_mode module parameter
from a per-device atomic event handler without synchronization?

Since appletb_tb_def_mode is a global variable exported as read-only,
modifying it dynamically could violate its sysfs contract and introduce a
data race if multiple keyboards are used concurrently.

This would also improperly alter the default mode for all current and future
connected devices.

> +
> +					appletb_kbd_set_mode(kbd,
> +						appletb_tb_def_mode);

[Severity: High]
This is a pre-existing issue, but does calling appletb_kbd_set_mode() from
this atomic context cause a kernel panic?

The appletb_kbd_inp_event handler is invoked by the input subsystem with
the dev->event_lock spinlock held and interrupts disabled.

appletb_kbd_set_mode() calls hid_hw_power() and hid_hw_request(). For USB
HID devices, hid_hw_power() may sleep and will warn in atomic context,
potentially leading to a scheduling while atomic panic.

> +
> +					kbd->saved_mode = appletb_tb_def_mode;
> +					kbd->last_fn_press = 0;
> +
> +					return;
> +				}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526154954.29002-1-gargaditya08@proton.me?part=1