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

[sashiko-bot@kernel.org]

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