[STABLE v6.1-v5.10 1/1] HID: logitech-hidpp: fix race condition when accessing stale stack pointer

[STABLE v6.1-v5.10 1/1] HID: logitech-hidpp: fix race condition when accessing stale stack pointer

[Lee Jones]

From: Benoît Sevens <bsevens@google.com>

commit e2aaf2d3ad92ac4a8afa6b69ad4c38e7747d3d6e upstream.

The driver uses hidpp->send_receive_buf to point to a stack-allocated
buffer in the synchronous command path (__do_hidpp_send_message_sync).
However, this pointer is not cleared when the function returns.

If an event is processed (e.g. by a different thread) while the
send_mutex is held by a new command, but before that command has
updated send_receive_buf, the handler (hidpp_raw_hidpp_event) will
observe that the mutex is locked and dereference the stale pointer.

This results in an out-of-bounds access on a different thread's kernel
stack (or a NULL pointer dereference on the very first command).

Fix this by:
1. Clearing hidpp->send_receive_buf to NULL before releasing the mutex
   in the synchronous command path.
2. Moving the assignment of the local 'question' and 'answer' pointers
   inside the mutex_is_locked() block in the handler, and adding
   a NULL check before dereferencing.

Fixes: 2f31c5252910 ("HID: Introduce hidpp, a module to handle Logitech hid++ devices")
Cc: stable@vger.kernel.org
Signed-off-by: Benoît Sevens <bsevens@google.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
[Lee: Clear hidpp->send_receive_buf at exit label in hidpp_send_message_sync()
      as __do_hidpp_send_message_sync() was split out later in 60165ab774cb]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index c65b5f004bac..4f597804c551 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -316,6 +316,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 	}
 
 exit:
+	hidpp->send_receive_buf = NULL;
 	mutex_unlock(&hidpp->send_mutex);
 	return ret;
 
@@ -3621,8 +3622,7 @@ static int hidpp_input_configured(struct hid_device *hdev,
 static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		int size)
 {
-	struct hidpp_report *question = hidpp->send_receive_buf;
-	struct hidpp_report *answer = hidpp->send_receive_buf;
+	struct hidpp_report *question, *answer;
 	struct hidpp_report *report = (struct hidpp_report *)data;
 	int ret;
 
@@ -3631,6 +3631,12 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 	 * previously sent command.
 	 */
 	if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
+		question = hidpp->send_receive_buf;
+		answer = hidpp->send_receive_buf;
+
+		if (!question)
+			return 0;
+
 		/*
 		 * Check for a correct hidpp20 answer or the corresponding
 		 * error
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [STABLE v6.1-v5.10 1/1] HID: logitech-hidpp: fix race condition when accessing stale stack pointer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid-logitech-hidpp: data race in hidpp_raw_hidpp_event() allows NULL pointer deref
--

commit d3703221a099da6d388bec38af269fe1cd97ac2c
Author: Benoît Sevens <bsevens@google.com>

    HID: logitech-hidpp: fix race condition when accessing stale stack pointer

    The driver uses a stack-allocated buffer in the synchronous command path but
    does not clear the pointer when returning. This patch clears the pointer and
    checks for NULL in the event handler to prevent out-of-bounds access.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 8a2aac18dcc51..f584c1007169c 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -322,6 +322,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
>  	}
>  
>  exit:
> +	hidpp->send_receive_buf = NULL;
>  	mutex_unlock(&hidpp->send_mutex);
>  	return ret;
>  

[ ... ]

> @@ -3590,6 +3590,12 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>  	 * previously sent command.
>  	 */
>  	if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
> +		question = hidpp->send_receive_buf;
> +		answer = hidpp->send_receive_buf;

[Severity: High]
Can these sequential lockless reads of hidpp->send_receive_buf allow a data
race?

Because hidpp_raw_hidpp_event() does not hold send_mutex when checking
mutex_is_locked(), another thread running hidpp_send_message_sync() could
modify hidpp->send_receive_buf concurrently.

If hidpp_send_message_sync() executes:

    hidpp->send_receive_buf = NULL;

exactly between the assignments to question and answer, could question
receive a non-NULL pointer while answer receives NULL?

> +
> +		if (!question)
> +			return 0;
> +

If the second read yields NULL for answer, the question check will pass.
Then, if the report matches, the handler will execute:

    *answer = *report;

Could this result in a NULL pointer dereference and panic the kernel?
Would it be safer to read hidpp->send_receive_buf exactly once,
perhaps with READ_ONCE(), and assign the result to both question and
answer?

>  		/*
>  		 * Check for a correct hidpp20 answer or the corresponding
>  		 * error

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922081615.821827-1-lee@kernel.org?part=1

Re: [STABLE v6.1-v5.10 1/1] HID: logitech-hidpp: fix race condition when accessing stale stack pointer

[Sasha Levin]

> [Lee: Clear hidpp->send_receive_buf at exit label in hidpp_send_message_sync()
>       as __do_hidpp_send_message_sync() was split out later in 60165ab774cb]

Queued for 6.1, 5.15 and 5.10, thanks.

-- 
Thanks,
Sasha