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

[sashiko-bot@kernel.org]

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