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

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

[Manish Khadka]

appleir_remove() runs hid_hw_stop() before timer_delete_sync().
hid_hw_stop() synchronously unregisters the HID input device via
hid_disconnect() -> hidinput_disconnect() -> input_unregister_device(),
which drops the last reference and frees the underlying input_dev when
no userspace handle holds it open.

key_up_tick() reads appleir->input_dev and calls input_report_key() /
input_sync() on it.  The timer is armed from appleir_raw_event() with
a HZ/8 (~125 ms) timeout on every keydown and key-repeat report.  If a
key was pressed shortly before the device is disconnected, the timer
can fire after hid_hw_stop() has freed input_dev but before
timer_delete_sync() drains it:

  CPU0                                CPU1
  appleir_raw_event()
    mod_timer(...)                    <- timer armed for 125 ms
  ...
  USB disconnect
  appleir_remove()
    hid_hw_stop()
      hid_disconnect()
        hidinput_disconnect()
          input_unregister_device()   <- input_dev freed
                                        key_up_tick()
                                          key_up()
                                            input_report_key(
                                              appleir->input_dev, ...)
                                                            ^^^^^^^^^ UAF
    timer_delete_sync()               <- too late

A simple reorder is not sufficient.  Putting timer_delete_sync() first
still leaves a window where a USB URB completion (raw_event) running
during hid_hw_stop() can call mod_timer() and re-arm the timer, which
then fires after hidinput_disconnect() has freed input_dev.

Introduce a 'removing' flag on struct appleir, gated by the existing
spinlock.  appleir_remove() sets the flag under the lock and then
drains the timer; appleir_raw_event() and key_up_tick() bail out
early if the flag is set, so no path can arm or run the timer after
remove() has drained it.

The keyrepeat branch of appleir_raw_event() previously called
key_down() and mod_timer() without holding the spinlock; take it now
so the flag check is well-defined.  This incidentally closes a
pre-existing read-side race on appleir->current_key in that branch.

This bug is structurally a sibling of commit 4db2af929279 ("HID:
appletb-kbd: fix UAF in inactivity-timer cleanup path") and has been
present since the driver was introduced.

Fixes: 9a4a5574ce42 ("HID: appleir: add support for Apple ir devices")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---
 drivers/hid/hid-appleir.c | 40 ++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-appleir.c b/drivers/hid/hid-appleir.c
index 5e8ced7bc05a..0e51f1e16653 100644
--- a/drivers/hid/hid-appleir.c
+++ b/drivers/hid/hid-appleir.c
@@ -109,9 +109,10 @@ struct appleir {
 	struct hid_device *hid;
 	unsigned short keymap[ARRAY_SIZE(appleir_key_table)];
 	struct timer_list key_up_timer;	/* timer for key up */
-	spinlock_t lock;		/* protects .current_key */
+	spinlock_t lock;		/* protects .current_key, .removing */
 	int current_key;		/* the currently pressed key */
 	int prev_key_idx;		/* key index in a 2 packets message */
+	bool removing;			/* set during teardown; gates input_dev access */
 };
 
 static int get_key(int data)
@@ -172,7 +173,7 @@ static void key_up_tick(struct timer_list *t)
 	unsigned long flags;
 
 	spin_lock_irqsave(&appleir->lock, flags);
-	if (appleir->current_key) {
+	if (!appleir->removing && appleir->current_key) {
 		key_up(hid, appleir, appleir->current_key);
 		appleir->current_key = 0;
 	}
@@ -195,6 +196,10 @@ static int appleir_raw_event(struct hid_device *hid, struct hid_report *report,
 		int index;
 
 		spin_lock_irqsave(&appleir->lock, flags);
+		if (appleir->removing) {
+			spin_unlock_irqrestore(&appleir->lock, flags);
+			goto out;
+		}
 		/*
 		 * If we already have a key down, take it up before marking
 		 * this one down
@@ -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;
 	}
 
@@ -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);
+	hid_hw_stop(hid);
 }
 
 static const struct hid_device_id appleir_devices[] = {
-- 
2.43.0

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

[sashiko-bot]

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

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

[Manish Khadka]

appleir_remove() runs hid_hw_stop() before timer_delete_sync().
hid_hw_stop() synchronously unregisters the HID input device via
hid_disconnect() -> hidinput_disconnect() -> input_unregister_device(),
which drops the last reference and frees the underlying input_dev when
no userspace handle holds it open.

key_up_tick() reads appleir->input_dev and calls input_report_key() /
input_sync() on it.  The timer is armed from appleir_raw_event() with
a HZ/8 (~125 ms) timeout on every keydown and key-repeat report.  If a
key was pressed shortly before the device is disconnected, the timer
can fire after hid_hw_stop() has freed input_dev but before the
teardown drains it.

A simple reorder is not sufficient.  Putting the timer drain first
still leaves a window where a USB URB completion (raw_event) running
during hid_hw_stop() can call mod_timer() and re-arm the timer, which
then fires after hidinput_disconnect() has freed input_dev.  The same
URB-completion window also lets raw_event() reach key_up(), key_down()
and battery_flat() directly, all of which dereference
appleir->input_dev.

Introduce a 'removing' flag on struct appleir, gated by the existing
spinlock.  appleir_remove() sets the flag under the lock and then
shuts down the timer with timer_shutdown_sync(), which both drains any
in-flight callback and permanently disables further mod_timer() calls.
appleir_raw_event() and key_up_tick() bail out early if the flag is
set, so no path can arm or run the timer, or dereference
appleir->input_dev, after remove() has started tearing down.

The keyrepeat and flatbattery branches of appleir_raw_event()
previously called into the input layer without holding the spinlock;
take it now so the flag check is well-defined.  This incidentally
closes a pre-existing read-side race on appleir->current_key in the
keyrepeat branch.

This bug is structurally a sibling of commit 4db2af929279 ("HID:
appletb-kbd: fix UAF in inactivity-timer cleanup path") and has been
present since the driver was introduced.

Fixes: 9a4a5574ce42 ("HID: appleir: add support for Apple ir devices")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---

v1 -> v2:
  - Address Sashiko AI review feedback:
    * [Critical] Gate the flatbattery branch in appleir_raw_event()
      under the existing spinlock + removing flag so battery_flat(),
      which does dev_err(&appleir->input_dev->dev, ...), cannot UAF
      during hid_hw_stop().
    * [Medium] Use timer_shutdown_sync() instead of timer_delete_sync()
      so the timer is permanently disabled in addition to drained,
      providing belt-and-suspenders against any future arming site
      that bypasses the removing flag.
  Thanks to Sashiko AI review for both points.
 drivers/hid/hid-appleir.c | 45 ++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-appleir.c b/drivers/hid/hid-appleir.c
index 5e8ced7bc05a..adaa44a858ed 100644
--- a/drivers/hid/hid-appleir.c
+++ b/drivers/hid/hid-appleir.c
@@ -109,9 +109,10 @@ struct appleir {
 	struct hid_device *hid;
 	unsigned short keymap[ARRAY_SIZE(appleir_key_table)];
 	struct timer_list key_up_timer;	/* timer for key up */
-	spinlock_t lock;		/* protects .current_key */
+	spinlock_t lock;		/* protects .current_key, .removing */
 	int current_key;		/* the currently pressed key */
 	int prev_key_idx;		/* key index in a 2 packets message */
+	bool removing;			/* set during teardown; gates input_dev access */
 };
 
 static int get_key(int data)
@@ -172,7 +173,7 @@ static void key_up_tick(struct timer_list *t)
 	unsigned long flags;
 
 	spin_lock_irqsave(&appleir->lock, flags);
-	if (appleir->current_key) {
+	if (!appleir->removing && appleir->current_key) {
 		key_up(hid, appleir, appleir->current_key);
 		appleir->current_key = 0;
 	}
@@ -195,6 +196,10 @@ static int appleir_raw_event(struct hid_device *hid, struct hid_report *report,
 		int index;
 
 		spin_lock_irqsave(&appleir->lock, flags);
+		if (appleir->removing) {
+			spin_unlock_irqrestore(&appleir->lock, flags);
+			goto out;
+		}
 		/*
 		 * If we already have a key down, take it up before marking
 		 * this one down
@@ -229,17 +234,25 @@ 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;
 	}
 
 	if (!memcmp(data, flatbattery, sizeof(flatbattery))) {
-		battery_flat(appleir);
+		spin_lock_irqsave(&appleir->lock, flags);
+		if (!appleir->removing)
+			battery_flat(appleir);
+		spin_unlock_irqrestore(&appleir->lock, flags);
 		/* Fall through */
 	}
 
@@ -318,8 +331,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);
+	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_shutdown_sync(&appleir->key_up_timer);
 	hid_hw_stop(hid);
-	timer_delete_sync(&appleir->key_up_timer);
 }
 
 static const struct hid_device_id appleir_devices[] = {
-- 
2.43.0