| 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
| ||
|
Date: Mon, 13 Dec 2021 14:36:12 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: lianzhi chang <changlianzhi@...ontech.com>
Cc: linux-kernel@...r.kernel.org, dmitry.torokhov@...il.com,
jirislaby@...nel.org, andriy.shevchenko@...ux.intel.com,
282827961@...com, kernel test robot <lkp@...el.com>,
Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH v20] tty: Fix the keyboard led light display problem
On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> desktop or switch the system from desktop to tty. After the switch is
> completed, it is found that the state of the keyboard lock is
> inconsistent with the state of the keyboard Led light.The reasons are
> as follows:
>
> * The desktop environment (Xorg and other services) is bound to a tty
> (assuming it is tty1), and the kb->kbdmode attribute value of tty1
> will be set to VC_OFF. According to the current code logic, in the
> desktop environment, the values of ledstate and kb->ledflagstate
> of tty1 will not be modified anymore, so they are always 0.
>
> * When switching between each tty, the final value of ledstate set by
> the previous tty is compared with the kb->ledflagstate value of the
> current tty to determine whether to set the state of the keyboard
> light. The process of switching between desktop and tty is also the
> process of switching between tty1 and other ttys. There are two
> situations:
>
> - (1) In the desktop environment, tty1 will not set the ledstate,
> which will cause when switching from the desktop to other ttys,
> if the desktop lights up the keyboard's led, after the switch is
> completed, the keyboard's led light will always be on;
>
> - (2) When switching from another tty to the desktop, this
> mechanism will trigger tty1 to set the led state. If other tty
> lights up the led of the keyboard before switching to the desktop,
> the led will be forcibly turned off. This situation should
> be avoided.
>
> * Current patch explanation:When VT is switched,the keyboard LED
> status will be set to the current tty saved value;the value of
> kb->kbdledctl can be used to confirm whether the current VT
> can change the status of the keyboard led light;kb->kbdledctl
> is a new addition,you can use ioctl get or set.
>
> Signed-off-by: lianzhi chang <changlianzhi@...ontech.com>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> ---
>
> v20:
> New solution: kbd_struct adds a new "kbdledctl" attribute,
> which can be obtained or changed through ioctl;
> "kbdledctl" is used to determine whether the current VT
> can set the keyboard led light;
>
> drivers/tty/vt/keyboard.c | 58 +++++++++++++++++++++++++++++++++++++++
> drivers/tty/vt/vt_ioctl.c | 12 ++++++++
> include/linux/kbd_kern.h | 4 +++
> include/linux/vt_kern.h | 2 ++
> include/uapi/linux/kd.h | 7 ++++-
> 5 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..248c8d790d91 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -153,6 +153,7 @@ static int shift_state = 0;
>
> static unsigned int ledstate = -1U; /* undefined */
> static unsigned char ledioctl;
> +static bool vt_switch;
This is an odd name for a variable that is there for led issues, right?
> /*
> * Notifier list for console keyboard events
> @@ -414,6 +415,12 @@ void vt_set_leds_compute_shiftstate(void)
> {
> unsigned long flags;
>
> + /*
> + * When switching VT,according to the value of vt_switch,
Extra space after ',' please.
> + * judge whether it is necessary to force the keyboard light
> + * state to be issued.
> + */
> + vt_switch = true;
> set_leds();
How does this new variable have anything to do with the set_leds() call?
>
> spin_lock_irqsave(&kbd_event_lock, flags);
> @@ -1247,14 +1254,24 @@ void vt_kbd_con_stop(unsigned int console)
> */
> static void kbd_bh(struct tasklet_struct *unused)
> {
> + struct kbd_struct *kb;
> unsigned int leds;
> unsigned long flags;
>
> + kb = kbd_table + fg_console;
So an pointer to an array? Raw pointer math is odd.
And no bounds checking?
> + if (kb->kbdledctl == VC_LEDCTL_OFF)
> + return;
Why exit now?
> +
> spin_lock_irqsave(&led_lock, flags);
> leds = getleds();
> leds |= (unsigned int)kbd->lockstate << 8;
> spin_unlock_irqrestore(&led_lock, flags);
>
> + if (vt_switch) {
> + ledstate = ~leds;
> + vt_switch = false;
> + }
Please explain what you are doing here with vt_switch.
> +
> if (leds != ledstate) {
> kbd_propagate_led_state(ledstate, leds);
> ledstate = leds;
> @@ -1857,6 +1874,35 @@ int vt_do_kdskbmode(unsigned int console, unsigned int arg)
> return ret;
> }
>
> +/**
> + * vt_do_kdskbledctl
> + * @console: the console to use
> + * @arg: the requested mode
> + *
> + * Whether to allow the current vt to change the
> + * keyboard light
> + */
> +int vt_do_kdskbledctl(unsigned int console, unsigned int arg)
> +{
> + struct kbd_struct *kb = &kbd_table[console];
> + int ret = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&kbd_event_lock, flags);
> + switch (arg) {
> + case K_LEDCTL_ON:
> + kb->kbdledctl = VC_LEDCTL_ON;
> + break;
> + case K_LEDCTL_OFF:
> + kb->kbdledctl = VC_LEDCTL_OFF;
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + spin_unlock_irqrestore(&kbd_event_lock, flags);
> + return ret;
> +}
> +
> /**
> * vt_do_kdskbmeta - set keyboard meta state
> * @console: the console to use
> @@ -2157,6 +2203,18 @@ int vt_do_kdgkbmode(unsigned int console)
> }
> }
>
> +int vt_do_kdgkbledctl(unsigned int console)
> +{
> + struct kbd_struct *kb = &kbd_table[console];
> + /* This is a spot read so needs no locking */
Blank line before comment?
And what do you mean by "spot read"?
> + switch (kb->kbdledctl) {
> + case VC_LEDCTL_ON:
> + return K_LEDCTL_ON;
> + case VC_LEDCTL_OFF:
> + return K_LEDCTL_OFF;
> + }
Why does usespace need to know this value? What will it do with it?
> +}
> +
> /**
> * vt_do_kdgkbmeta - report meta status
> * @console: console to report
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 3639bb6dc372..c78bd452af55 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -405,6 +405,18 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> case KDGKBMODE:
> return put_user(vt_do_kdgkbmode(console), (int __user *)arg);
>
> + case KDSKBLEDCTL:
> + if (!perm)
> + return -EPERM;
> + ret = vt_do_kdskbledctl(console, arg);
> + if (ret)
> + return ret;
> + tty_ldisc_flush(tty);
> + break;
> +
> + case KDGKBLEDCTL:
> + return put_user(vt_do_kdgkbledctl(console), (int __user *)arg);
> +
> /* this could be folded into KDSKBMODE, but for compatibility
> reasons it is not so easy to fold KDGKBMETA into KDGKBMODE */
> case KDSKBMETA:
> diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
> index c40811d79769..769b946c2cf9 100644
> --- a/include/linux/kbd_kern.h
> +++ b/include/linux/kbd_kern.h
> @@ -32,6 +32,10 @@ struct kbd_struct {
> #define VC_CTRLRLOCK KG_CTRLR /* ctrlr lock mode */
> unsigned char slockstate; /* for `sticky' Shift, Ctrl, etc. */
>
> + unsigned char kbdledctl:1； /*Whether to allow to control the led of the keyboard */
> +#define VC_LEDCTL_ON 0 /* VT can set the keyboard light */
> +#define VC_LEDCTL_OFF 1 /* Prohibit VT to set the keyboard light */
> +
> unsigned char ledmode:1;
> #define LED_SHOW_FLAGS 0 /* traditional state */
> #define LED_SHOW_IOCTL 1 /* only change leds upon ioctl */
> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> index b5ab452fca5b..67bdf3eddb5a 100644
> --- a/include/linux/vt_kern.h
> +++ b/include/linux/vt_kern.h
> @@ -149,6 +149,7 @@ void hide_boot_cursor(bool hide);
> /* keyboard provided interfaces */
> int vt_do_diacrit(unsigned int cmd, void __user *up, int eperm);
> int vt_do_kdskbmode(unsigned int console, unsigned int arg);
> +int vt_do_kdskbledctl(unsigned int console, unsigned int arg);
> int vt_do_kdskbmeta(unsigned int console, unsigned int arg);
> int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc,
> int perm);
> @@ -157,6 +158,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
> int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm);
> int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm);
> int vt_do_kdgkbmode(unsigned int console);
> +int vt_do_kdgkbledctl(unsigned int console);
> int vt_do_kdgkbmeta(unsigned int console);
> void vt_reset_unicode(unsigned int console);
> int vt_get_shift_state(void);
> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> index ee929ece4112..138d636adf3e 100644
> --- a/include/uapi/linux/kd.h
> +++ b/include/uapi/linux/kd.h
> @@ -86,6 +86,11 @@ struct unimapinit {
> #define KDGKBMODE 0x4B44 /* gets current keyboard mode */
> #define KDSKBMODE 0x4B45 /* sets current keyboard mode */
>
> +#define K_LEDCTL_ON 0x00
> +#define K_LEDCTL_OFF 0x01
As Dan points out, these should be the other way around.
> +#define KDGKBLEDCTL 0x4B73 /* set whether to allow control of keyboard lights */
> +#define KDSKBLEDCTL 0x4B74 /* get whether the keyboard light is currently allowed to be set */
What userspace code is going to use these new ioctls?
I still don't understand the problem that this is supposed to be
solving. How can we have never had a problem until now with regards to
LED settings on keyboards? What commit caused this to change? Has it
always been broken for 30 years and no one noticed?
thanks,
greg k-h
Powered by blists - more mailing lists