[PATCH] vt: keyboard: serialize KDGETLED with keyboard_tasklet

[PATCH] vt: keyboard: serialize KDGETLED with keyboard_tasklet

[Cen Zhang]

KDGETLED reaches getledstate() and samples ledstate without serializing
against keyboard_tasklet.

That is problematic because kbd_bh() temporarily stores ~leds into
ledstate during a VT switch to force LED propagation before committing
the final value.  An unlocked KDGETLED can therefore observe that
internal sentinel instead of the last committed LED state.

Other ledstate readers in this file already quiesce keyboard_tasklet
before sampling the value.  Do the same in getledstate() so the ioctl
only returns a stable snapshot.

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 drivers/tty/vt/keyboard.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 13bc048f45e86..671ad4a5cb64c 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1127,7 +1127,13 @@ static void kbd_init_leds(void)
  */
 static unsigned char getledstate(void)
 {
-	return ledstate & 0xff;
+	unsigned char leds;
+
+	tasklet_disable(&keyboard_tasklet);
+	leds = ledstate & 0xff;
+	tasklet_enable(&keyboard_tasklet);
+
+	return leds;
 }
 
 void setledstate(struct kbd_struct *kb, unsigned int led)
-- 
2.43.0

Re: [PATCH] vt: keyboard: serialize KDGETLED with keyboard_tasklet

[Greg KH]

On Sun, May 10, 2026 at 11:13:48AM +0800, Cen Zhang wrote:
> KDGETLED reaches getledstate() and samples ledstate without serializing
> against keyboard_tasklet.
> 
> That is problematic because kbd_bh() temporarily stores ~leds into
> ledstate during a VT switch to force LED propagation before committing
> the final value.  An unlocked KDGETLED can therefore observe that
> internal sentinel instead of the last committed LED state.

Why is that an issue?

> Other ledstate readers in this file already quiesce keyboard_tasklet
> before sampling the value.  Do the same in getledstate() so the ioctl
> only returns a stable snapshot.

stable for "what" exactly?  This is a snapshot in time, be it before or
after it changes is not always going to really matter here, as it can
change right after you "enable" the tasklet, right?

thanks,

greg k-h

Re: [PATCH] vt: keyboard: serialize KDGETLED with keyboard_tasklet

[Cen Zhang]

Hi Greg,

> On Sun, May 10, 2026 at 06:22:xxPM +0200, Greg KH wrote:

> Why is that an issue?

Thanks, that's a fair point. The issue I was trying to describe is not
that KDGETLED can race with a normal LED state transition and return
either the old or the new value. That part is just the usual snapshot
semantics, and the value can of course change immediately after the ioctl
returns.

The specific case I was concerned about is the VT-switch force-update
path. vt_set_leds_compute_shiftstate() sets vt_switch and schedules
keyboard_tasklet, and then kbd_bh() does:

    if (vt_switch) {
            ledstate = ~leds;
            vt_switch = false;
    }

    if (leds != ledstate) {
            kbd_propagate_led_state(ledstate, leds);
            ledstate = leds;
    }

That first assignment is only an internal old-state sentinel to force LED
propagation when switching VTs. It is not a committed LED state intended
to be returned to userspace. Since KDGETLED currently returns
ledstate & 0xff without quiescing the tasklet, it can expose that
temporary complemented value. For example, if the real LED state is
LED_NUM only, a read in that window can return the complemented low byte
instead of 0x02.

> stable for "what" exactly? This is a snapshot in time, be it before or
> after it changes is not always going to really matter here, as it can
> change right after you "enable" the tasklet, right?

Yes, I agree. "stable snapshot" was too broad a description.

By "stable" I only meant "not while kbd_bh() is in the middle of using
that internal force-update value". tasklet_disable() would make KDGETLED
observe either the old value before the tasklet runs, or the committed
value after it finishes. It does not make the value stable after
tasklet_enable(), and the changelog should not imply that.

This is a small userspace-visible consistency issue, not a memory-safety
problem. If this behavior is considered acceptable for KDGETLED, I can
drop the patch; otherwise I can send a v2 with a narrower changelog.

Best regards,
Cen

Re: [PATCH] vt: keyboard: serialize KDGETLED with keyboard_tasklet

[Greg KH]

On Mon, May 11, 2026 at 12:43:26AM +0800, Cen Zhang wrote:
> Hi Greg,
> 
> > On Sun, May 10, 2026 at 06:22:xxPM +0200, Greg KH wrote:
> 
> > Why is that an issue?
> 
> Thanks, that's a fair point. The issue I was trying to describe is not
> that KDGETLED can race with a normal LED state transition and return
> either the old or the new value. That part is just the usual snapshot
> semantics, and the value can of course change immediately after the ioctl
> returns.
> 
> The specific case I was concerned about is the VT-switch force-update
> path. vt_set_leds_compute_shiftstate() sets vt_switch and schedules
> keyboard_tasklet, and then kbd_bh() does:
> 
>     if (vt_switch) {
>             ledstate = ~leds;
>             vt_switch = false;
>     }
> 
>     if (leds != ledstate) {
>             kbd_propagate_led_state(ledstate, leds);
>             ledstate = leds;
>     }
> 
> That first assignment is only an internal old-state sentinel to force LED
> propagation when switching VTs. It is not a committed LED state intended
> to be returned to userspace. Since KDGETLED currently returns
> ledstate & 0xff without quiescing the tasklet, it can expose that
> temporary complemented value. For example, if the real LED state is
> LED_NUM only, a read in that window can return the complemented low byte
> instead of 0x02.
> 
> > stable for "what" exactly? This is a snapshot in time, be it before or
> > after it changes is not always going to really matter here, as it can
> > change right after you "enable" the tasklet, right?
> 
> Yes, I agree. "stable snapshot" was too broad a description.
> 
> By "stable" I only meant "not while kbd_bh() is in the middle of using
> that internal force-update value". tasklet_disable() would make KDGETLED
> observe either the old value before the tasklet runs, or the committed
> value after it finishes. It does not make the value stable after
> tasklet_enable(), and the changelog should not imply that.
> 
> This is a small userspace-visible consistency issue, not a memory-safety
> problem. If this behavior is considered acceptable for KDGETLED, I can
> drop the patch; otherwise I can send a v2 with a narrower changelog.

In the past decades of use, has anyone every noticed or complained about
this consitency issue?  If not, I would recommend not worrying about it :)

thanks,

greg k-h

Re: [PATCH] vt: keyboard: serialize KDGETLED with keyboard_tasklet

[Cen Zhang]

Hi Greg,

> In the past decades of use, has anyone every noticed or complained about
> this consitency issue?  If not, I would recommend not worrying about it :)
>

Understood, thanks. I will drop this patch.

Thank you for the review and guidance.

Best regards,
Cen