Re: [PATCH] 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:
- [High] Zero-initialized timestamp causes single keypresses to be erroneously treated as double-presses during the first 5 minutes of system uptime.
- [Medium] Dynamic, lockless modification of a read-only global module parameter from a per-device event handler.
- [Low] The module parameter `double_press_switch_time` is defined as a signed integer but used as an unsigned value, allowing negative values to cause abnormally large timeouts.

Pre-existing issues:
- [High] The input event handler calls a function that can sleep, leading to a schedule-while-atomic bug because input handlers execute with spinlocks held and interrupts disabled.
--

commit ff52ac5f6303e0ff3ca2a68218cce8a8beb04bf2
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 within a configurable
time interval.

> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..4f900338b5871 100644
> --- a/drivers/hid/hid-appletb-kbd.c
> +++ b/drivers/hid/hid-appletb-kbd.c
> @@ -56,6 +56,12 @@ static int appletb_tb_idle_timeout = 15;
>  module_param_named(idle_timeout, appletb_tb_idle_timeout, int, 0644);
>  MODULE_PARM_DESC(idle_timeout, "Idle timeout in sec");
>  
> +static int appletb_tb_double_press_switch_layers;
> +module_param_named(double_press_switch_time,
> +		   appletb_tb_double_press_switch_layers, int, 0644);

[Severity: Low]
If this parameter is set to a negative value by root at runtime, will it
be implicitly cast to a massive unsigned value when passed to
msecs_to_jiffies() below?

This could result in an extremely large timeout (e.g. 49 days) where every
sequential press acts as a double-press. Should this parameter be typed as
a uint or have bounds checking applied?

[ ... ]
> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
[ ... ]
> +			if (appletb_tb_double_press_switch_layers) {
> +				unsigned long now = jiffies;
> +
> +				if (time_before(now, kbd->last_fn_press +
> +					msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {

[Severity: High]
Does this logic correctly handle the first 5 minutes of system uptime?

Because the kernel initializes jiffies to a large negative value (typically
-300 * HZ to detect wraparound bugs), and kbd->last_fn_press is initialized
and reset to 0, won't time_before() evaluate to unconditionally true during
early boot?

This might cause every single keypress to be falsely treated as a
double-press until the system has been running long enough for jiffies to
become positive.

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

[Severity: Medium]
Is it safe to modify appletb_tb_def_mode here?

This variable is exposed as a module parameter with 0444 (read-only)
permissions. Modifying it programmatically alters the default state for all
current and future connected instances of the device based on a single
device's event.

Should this default mode be managed as a per-device state inside
struct appletb_kbd instead?

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

[Severity: High]
This is a pre-existing issue, but does calling appletb_kbd_set_mode() here
cause a scheduling while atomic bug?

This handler executes with dev->event_lock held and interrupts disabled.
However, appletb_kbd_set_mode() calls hid_hw_power(..., PM_HINT_FULLON),
which for USB HID devices invokes usbhid_power() and
usb_autopm_get_interface().

Since those functions lock a mutex and can sleep while waiting on power
management operations, calling them from an atomic context might cause a
kernel panic.

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