Re: [PATCH] HID: appleir: fix UAF on pending key_up_timer in remove()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unprotected access to `appleir->input_dev` in the `flatbattery` event path leads to a Use-After-Free during device teardown.
- [Medium] The teardown path uses `timer_delete_sync()` instead of the mandated `timer_shutdown_sync()` API.
--

commit 85e03ec2fd55b34b6351049c19c0b16cf580e59e
Author: Manish Khadka <maskmemanish@gmail.com>
Subject: HID: appleir: fix UAF on pending key_up_timer in remove()

This commit introduces a removing flag to prevent a use-after-free during
device teardown. It checks this flag in the event handler to avoid arming
the timer or accessing the input device after it has been freed by
hid_hw_stop().

> diff --git a/drivers/hid/hid-appleir.c b/drivers/hid/hid-appleir.c
> index 5e8ced7bc05a3..0e51f1e166531 100644
> --- a/drivers/hid/hid-appleir.c
> +++ b/drivers/hid/hid-appleir.c

[ ... ]

> @@ -229,12 +234,17 @@ static int appleir_raw_event(struct hid_device *hid, struct hid_report *report,
>  	appleir->prev_key_idx = 0;
>  
>  	if (!memcmp(data, keyrepeat, sizeof(keyrepeat))) {
> -		key_down(hid, appleir, appleir->current_key);
> -		/*
> -		 * Remote doesn't do key up, either pull them up, in the test
> -		 * above, or here set a timer which pulls them up after 1/8 s
> -		 */
> -		mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
> +		spin_lock_irqsave(&appleir->lock, flags);
> +		if (!appleir->removing) {
> +			key_down(hid, appleir, appleir->current_key);
> +			/*
> +			 * Remote doesn't do key up, either pull them up, in
> +			 * the test above, or here set a timer which pulls them
> +			 * up after 1/8 s
> +			 */
> +			mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
> +		}
> +		spin_unlock_irqrestore(&appleir->lock, flags);
>  		goto out;
>  	}

Does this leave the flatbattery branch unprotected? 

If a flat battery packet is received during the teardown window, execution
skips these guarded blocks and falls straight through to battery_flat()
which follows immediately after this hunk.

Looking at battery_flat(), it seems to unconditionally execute
dev_err(&appleir->input_dev->dev, ...), which dereferences the input_dev
pointer. Can this result in a use-after-free if it fires concurrently with
teardown?

[ ... ]

> @@ -318,8 +328,20 @@ static int appleir_probe(struct hid_device *hid, const struct hid_device_id *id)
>  static void appleir_remove(struct hid_device *hid)
>  {
>  	struct appleir *appleir = hid_get_drvdata(hid);
> -	hid_hw_stop(hid);
> +	unsigned long flags;
> +
> +	/*
> +	 * Mark the driver as tearing down so that any concurrent raw_event
> +	 * (e.g. from a USB URB completion that hid_hw_stop() has not yet
> +	 * killed) and the key_up_timer softirq stop touching input_dev
> +	 * before hid_hw_stop() frees it via hidinput_disconnect().
> +	 */
> +	spin_lock_irqsave(&appleir->lock, flags);
> +	appleir->removing = true;
> +	spin_unlock_irqrestore(&appleir->lock, flags);
> +
>  	timer_delete_sync(&appleir->key_up_timer);

Should this teardown path use timer_shutdown_sync() instead?

The timer subsystem guidelines mandate using timer_shutdown_sync() in
teardown paths to permanently prevent the timer from being re-armed.

> +	hid_hw_stop(hid);
>  }

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