[PATCH] Input: ipaq-micro-keys - fix potential deadlock

[PATCH] Input: ipaq-micro-keys - fix potential deadlock

[Dmitry Torokhov]

The driver acquires the micro->lock spinlock in process context (in
micro_key_start() and micro_key_stop()) without disabling interrupts.
However, this lock is also acquired in hardirq context by the MFD core
rx handler (micro_rx_msg()) which is called from the serial ISR.

This can lead to a lock inversion deadlock if the interrupt fires on the
same CPU while the process context holds the lock.

Fix this by using guard(spinlock_irq) instead of guard(spinlock) in
micro_key_start() and micro_key_stop() to disable interrupts while
holding the lock.

Reported-by: sashiko-bot@kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/ipaq-micro-keys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/ipaq-micro-keys.c b/drivers/input/keyboard/ipaq-micro-keys.c
index ca7ec054b1ce..63d589253e46 100644
--- a/drivers/input/keyboard/ipaq-micro-keys.c
+++ b/drivers/input/keyboard/ipaq-micro-keys.c
@@ -54,7 +54,7 @@ static void micro_key_receive(void *data, int len, unsigned char *msg)
 
 static void micro_key_start(struct ipaq_micro_keys *keys)
 {
-	guard(spinlock)(&keys->micro->lock);
+	guard(spinlock_irq)(&keys->micro->lock);
 
 	keys->micro->key = micro_key_receive;
 	keys->micro->key_data = keys;
@@ -62,7 +62,7 @@ static void micro_key_start(struct ipaq_micro_keys *keys)
 
 static void micro_key_stop(struct ipaq_micro_keys *keys)
 {
-	guard(spinlock)(&keys->micro->lock);
+	guard(spinlock_irq)(&keys->micro->lock);
 
 	keys->micro->key = NULL;
 	keys->micro->key_data = NULL;
-- 
2.54.0.1099.g489fc7bff1-goog


-- 
Dmitry

Re: [PATCH] Input: ipaq-micro-keys - fix potential deadlock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] ipaq_micro_str() fails to null-terminate a stack-allocated string before passing it to kstrdup().
- [Medium] micro_key_receive() accesses the message payload without checking if the length is greater than zero.
- [Medium] Potential off-by-one error in keycode array indexing.
--

commit cc960c88f9bd89346f2cb6d8461bbe44bd01c18b
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: ipaq-micro-keys - fix potential deadlock

This commit fixes a lock inversion deadlock in the ipaq-micro-keys driver.
It updates micro_key_start() and micro_key_stop() to use guard(spinlock_irq)
instead of guard(spinlock), ensuring interrupts are disabled while the lock
is held in process context.

> diff --git a/drivers/input/keyboard/ipaq-micro-keys.c b/drivers/input/keyboard/ipaq-micro-keys.c
> index ca7ec054b1cef..63d589253e465 100644
> --- a/drivers/input/keyboard/ipaq-micro-keys.c
> +++ b/drivers/input/keyboard/ipaq-micro-keys.c

[ ... ]

> @@ -54,7 +54,7 @@ static void micro_key_receive(void *data, int len, unsigned char *msg)

[Severity: Medium]
This isn't a bug introduced by this patch, but could this code read
uninitialized or stale memory if len is zero?

In micro_key_receive():

    down = 0x80 & msg[0];
    key  = 0x7f & msg[0];

The parent MFD driver parses serial messages and can validly produce a
payload with a length of 0. When this happens, micro_process_char() skips
writing to the buffer, leaving it with stale data. 

Since micro_key_receive() reads msg[0] without checking if len > 0, does
this cause the driver to report phantom key events based on old hardware
traffic?

[Severity: Medium]
This is a pre-existing issue, but does this array indexing have an
off-by-one error?

In micro_key_receive():

    if (key < ARRAY_SIZE(micro_keycodes)) {
        input_report_key(keys->input, keys->codes[key], down);
        input_sync(keys->input);
    }

If the hardware sends 1-based codes (for example, 1 for Record and 9
for Down), using the hardware key value directly as a zero-based index means
a value of 1 maps to index 1 instead of 0.

Furthermore, if the hardware sends 9 for the Down button, the event would be
dropped entirely since 9 < 9 evaluates to false. Should the index be
decremented before lookup if the device uses 1-based indexing?

>  
>  static void micro_key_start(struct ipaq_micro_keys *keys)
>  {
> -	guard(spinlock)(&keys->micro->lock);
> +	guard(spinlock_irq)(&keys->micro->lock);
>  
>  	keys->micro->key = micro_key_receive;
>  	keys->micro->key_data = keys;
> @@ -62,7 +62,7 @@ static void micro_key_start(struct ipaq_micro_keys *keys)
>  
>  static void micro_key_stop(struct ipaq_micro_keys *keys)
>  {
> -	guard(spinlock)(&keys->micro->lock);
> +	guard(spinlock_irq)(&keys->micro->lock);
>  
>  	keys->micro->key = NULL;
>  	keys->micro->key_data = NULL;

[Severity: High]
This is also a pre-existing issue in a related file, but does this code
cause an out-of-bounds stack read?

In drivers/mfd/ipaq-micro.c:ipaq_micro_str():

    for (i = 0; i < len / 2; i++)
        retstr[i] = wchar[i * 2];
    return kstrdup(retstr, GFP_KERNEL);

The array retstr is filled without an explicit null terminator. When
kstrdup() is called, it relies on strlen(), which will read past the valid
copied characters into uninitialized stack memory until it hits a random null
byte.

Could this leak kernel stack contents when the string is later printed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/aij-pfaKK-Nna7wf@google.com?part=1