linux-kernel - Re: [PATCH] input&tty: Fix the keyboard led light display problem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <63beeeac-f6c6-5780-a089-35e4e75cabb5@kernel.org>
Date:   Thu, 14 Oct 2021 06:12:07 +0200
From:   Jiri Slaby <jirislaby@...nel.org>
To:     lianzhi chang <changlianzhi@...ontech.com>,
        linux-kernel@...r.kernel.org
Cc:     dmitry.torokhov@...il.com, gregkh@...uxfoundation.org,
        andriy.shevchenko@...ux.intel.com, linux-input@...r.kernel.org,
        282827961@...com
Subject: Re: [PATCH] input&tty: Fix the keyboard led light display problem

On 14. 10. 21, 5:08, lianzhi chang wrote:
> Switching from the desktop environment to the tty environment,
> the state of the keyboard led lights and the state of the keyboard
> lock are inconsistent. This is because the attribute kb->kbdmode
> of the tty bound in the desktop environment (xorg) is set to
> VC_OFF, which causes the ledstate and kb->ledflagstate
> values of the bound tty to always be 0, which causes the switch
> from the desktop When to the tty environment, the LED light
> status is inconsistent with the keyboard lock status.
> 
> Signed-off-by: lianzhi chang <changlianzhi@...ontech.com>
> ---
>   drivers/input/input.c     |  7 ++++++-
>   drivers/tty/vt/keyboard.c | 22 +++++++++++++++++++++-
>   include/linux/kbd_kern.h  |  2 ++
>   3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index ccaeb2426385..41f6186a9cc4 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -25,6 +25,7 @@
>   #include <linux/rcupdate.h>
>   #include "input-compat.h"
>   #include "input-poller.h"
> +#include <linux/kbd_kern.h>
>   
>   MODULE_AUTHOR("Vojtech Pavlik <vojtech@...e.cz>");
>   MODULE_DESCRIPTION("Input core");
> @@ -472,8 +473,12 @@ void input_inject_event(struct input_handle *handle,
>   
>   		rcu_read_lock();
>   		grab = rcu_dereference(dev->grab);
> -		if (!grab || grab == handle)
> +		if (!grab || grab == handle) {
>   			input_handle_event(dev, type, code, value);
> +
> +			if (type == EV_LED && code < 3)

3 is a magic constant. In fact, you mean "code <= LED_SCROLLL", right?

> +				update_value_ledstate(code, value);
> +		}
>   		rcu_read_unlock();
>   
>   		spin_unlock_irqrestore(&dev->event_lock, flags);
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..005fa5cf2a67 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -1140,6 +1140,23 @@ static unsigned char getledstate(void)
>   	return ledstate & 0xff;
>   }
>   
> +void update_value_ledstate(int flag, int value)

Why is the int signed?

> +{
> +	if (ledstate == -1U)
> +		ledstate = 0;
> +
> +	if (flag == LED_NUML) {
> +		ledstate &= ~(1 << 1);

ledstate &= ~BIT(VC_NUMLOCK);

> +		ledstate |= value << 1;

What ensures that value is 0 or 1 here?

> +	} else if (flag == LED_CAPSL) {
> +		ledstate &= ~(1 << 2);
> +		ledstate |= value << 2;
> +	} else if (flag == LED_SCROLLL) {
> +		ledstate &= ~(1 << 0);
> +		ledstate |= value << 0;
> +	}

What about something like this (maybe ifs would be shorter, dunno, try it):
unsigned int bit;

switch (flag) {
case LED_NUML:
   bit = VC_NUMLOCK;
   break;
case LED_CAPSL:
   bit = VC_CAPSLOCK;
   break;
case LED_SCROLLL:
   bit = VC_SCROLLOCK;
   break;
default:
   WARN_ON_ONCE(1);
   return;
}

if (value)
   ledstate |= BIT(bit);
else
   ledstate &= ~BIT(BIT);

> +}
> +
>   void setledstate(struct kbd_struct *kb, unsigned int led)
>   {
>           unsigned long flags;
> @@ -1249,6 +1266,10 @@ static void kbd_bh(struct tasklet_struct *unused)
>   {
>   	unsigned int leds;
>   	unsigned long flags;
> +	struct kbd_struct *kb = kbd_table + fg_console;
> +
> +	if (kb->kbdmode == VC_OFF)
> +		return;

Can you explain why (not to me, to the commit log)?

>   	spin_lock_irqsave(&led_lock, flags);
>   	leds = getleds();
> @@ -1257,7 +1278,6 @@ static void kbd_bh(struct tasklet_struct *unused)
>   
>   	if (leds != ledstate) {
>   		kbd_propagate_led_state(ledstate, leds);
> -		ledstate = leds;

The same here. Why this doesn't break the current behaviour.

>   	}
>   }
>   
> diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
> index c40811d79769..36a3402658e6 100644
> --- a/include/linux/kbd_kern.h
> +++ b/include/linux/kbd_kern.h
> @@ -62,6 +62,8 @@ extern int kbd_init(void);
>   
>   extern void setledstate(struct kbd_struct *kbd, unsigned int led);
>   
> +extern void update_value_ledstate(int flag, int value);
> +
>   extern int do_poke_blanked_console;
>   
>   extern void (*kbd_ledfunc)(unsigned int led);
> 

thanks,
-- 
js
suse labs