Re: [PATCH v2 RESEND] 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 4 potential issue(s) to consider:

New issues:
- [Medium] The driver dynamically mutates a read-only global module parameter without synchronization.
- [Medium] Zero-initialization of `kbd->last_fn_press` paired with jiffies wrap logic causes spurious double-press detections during the first 5 minutes of system uptime.

Pre-existing issues:
- [High] Calling sleeping functions (`hid_hw_power`, `pm_runtime_get_sync`) from the atomic input event handler causes 'BUG: scheduling while atomic'.
- [High] Probe enables hardware events before setting driver data, causing a NULL pointer dereference.
--

commit e33a32d065a3f43246ae8063aed73e4573a2176b
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 whose
value depicts the time in milliseconds within which the fn key must be
pressed again to switch the default layer. If set to 0, it simply
disables this behaviour.

> 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

[ ... ]

> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
>  			      unsigned int code, int value)
>  {
>  	struct appletb_kbd *kbd = handle->private;
>  
>  	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)) {
> +
>  		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]
Since the system jiffies timer initializes to a negative value at boot, does
evaluating time_before() with a zero-initialized kbd->last_fn_press cause
spurious double-press detections during the first 5 minutes of system uptime?

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

[Severity: Medium]
Since appletb_tb_def_mode is a global module parameter with read-only
permissions, does dynamically reassigning it here violate its read-only
contract and globally modify the default layer for all device instances
instead of applying the state locally to the specific kbd instance?

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

[Severity: High]
This isn't a bug introduced by this patch, but does appletb_kbd_set_mode()
unconditionally call hid_hw_power(), which can sleep via pm_runtime_get_sync()?

Since the input core invokes appletb_kbd_inp_event() from atomic context with
the dev->event_lock spinlock held and local IRQs disabled, can this trigger a
scheduling while atomic bug?

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

[Severity: Medium]
Does explicitly resetting last_fn_press back to 0 here cause the spurious
evaluations to continue for subsequent presses within the first 5 minutes of
uptime?

> +
> +					return;
> +				}
> +
> +				kbd->last_fn_press = now;
> +			}
> +			if (appletb_tb_fn_toggle) {
> +				kbd->saved_mode = kbd->current_mode;
> +
> +				appletb_kbd_set_mode(kbd,
> +					appletb_switch_mode(kbd->current_mode));
> +			}
> +
>  		} else if (value == 0) {
> -			if (kbd->saved_mode != kbd->current_mode)
> +			if (appletb_tb_fn_toggle &&
> +			    kbd->saved_mode != kbd->current_mode)
>  				appletb_kbd_set_mode(kbd, kbd->saved_mode);
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does appletb_kbd_probe() enable hardware
events before setting the driver data?

drivers/hid/hid-appletb-kbd.c:appletb_kbd_probe() {
    ...
    ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
    ...
    hid_set_drvdata(hdev, kbd);
    ...
}

Once the hardware is opened, it can immediately begin delivering HID reports
which invoke appletb_kbd_hid_event(). Could appletb_kbd_hid_event() fetch an
uninitialized NULL pointer from hid_get_drvdata(hdev) and pass it to
reset_inactivity_timer(), causing a NULL pointer dereference?

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