[PATCH v2] tty: sysrq: reset the per-device key state when reparsing reset_seq

[PATCH v2] tty: sysrq: reset the per-device key state when reparsing reset_seq

[Zhan Xusheng]

sysrq_parse_reset_sequence() clears reset_seq_cnt, but reset_keybit is
only ever __set_bit() and reset_seq_len only ever incremented, and
nothing else clears either.  struct sysrq_state is allocated once per
input device in sysrq_connect() and lives until the device goes away, so
both accumulate every time the sequence is reparsed, which happens
whenever the reset_seq module parameter is written:

  # echo 56,29,111 > /sys/module/sysrq/parameters/reset_seq
  # echo 59,60     > /sys/module/sysrq/parameters/reset_seq

After the second write reset_keybit holds all five keys and reset_seq_len
is 5, so the combination that resets the machine is the union of the old
and the new one.  Holding just 59+60 counts 2 of 5 and does nothing.
Releasing one of the stale keys decrements reset_seq_cnt below zero.

Clear both along with reset_seq_cnt.

Doing only that would drop a cancellation the stale bits were providing
by accident: with sysrq_downtime_ms set, holding a full sequence arms
keyreset_timer, and the timer is only deleted on a press outside the
sequence or a release inside it.  With the old keys gone from
reset_keybit their release matches neither, and the armed reset survives
into orderly_reboot().  Delete the timer as well, since it belongs to the
sequence being replaced.

The boot paths are unaffected: the command line and
sysrq_of_get_keyreset_config() leave sysrq_reset_seq_version at 1, so a
freshly kzalloc'd state parses once with both fields already zero.

Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
Cc: stable@vger.kernel.org
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
v1: https://lore.kernel.org/all/20261016032732.2328852-1-zhanxusheng@xiaomi.com/

v1 -> v2:
 - Retitle from "Input:" to "tty: sysrq:" and add Cc: stable, per Greg KH.
 - Delete keyreset_timer when the sequence is redefined.  Clearing
   reset_keybit removes a cancellation the stale bits were providing by
   accident, so without this an armed reset would survive the release of
   the old keys.  Reported by the Sashiko bot.
 - Fix the description of the breakage.  reset_seq_cnt does reach
   reset_seq_len once every accumulated key is held, so the reset
   combination becomes the union of the old and the new sequence rather
   than stopping working.
 drivers/tty/sysrq.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index c2e4b31b699aa..013ad3a500703 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -689,6 +689,15 @@ static void sysrq_parse_reset_sequence(struct sysrq_state *state)
 	int i;
 	unsigned short key;
 
+	/*
+	 * An armed reset belongs to the sequence being replaced: once the
+	 * old keys are gone from reset_keybit their release no longer
+	 * cancels it.
+	 */
+	timer_delete(&state->keyreset_timer);
+
+	bitmap_zero(state->reset_keybit, KEY_CNT);
+	state->reset_seq_len = 0;
 	state->reset_seq_cnt = 0;
 
 	for (i = 0; i < sysrq_reset_seq_len; i++) {

base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
-- 
2.43.0

Re: [PATCH v2] tty: sysrq: reset the per-device key state when reparsing reset_seq

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tty: sysrq: TOCTOU race in param_array parsing permanently truncates reset sequence
--

commit d416bc3fc7ebb943414eab9dc3be1f59ade9fcc6
Author: Zhan Xusheng <zhanxusheng1024@gmail.com>

tty: sysrq: reset the per-device key state when reparsing reset_seq

sysrq_parse_reset_sequence() clears reset_seq_cnt, but reset_keybit is
only ever __set_bit() and reset_seq_len only ever incremented. Since
struct sysrq_state lives until the device goes away, both accumulate
every time the sequence is reparsed. This commit fixes this by clearing
both along with reset_seq_cnt and deleting the keyreset_timer when the
sequence is redefined.

> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index c2e4b31b699aa..013ad3a500703 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -689,6 +689,15 @@ static void sysrq_parse_reset_sequence(struct sysrq_state *state)
>  	int i;
>  	unsigned short key;
>  
> +	/*
> +	 * An armed reset belongs to the sequence being replaced: once the
> +	 * old keys are gone from reset_keybit their release no longer
> +	 * cancels it.
> +	 */
> +	timer_delete(&state->keyreset_timer);
> +
> +	bitmap_zero(state->reset_keybit, KEY_CNT);
> +	state->reset_seq_len = 0;

[Severity: Medium]
When a user writes a comma-separated list of keys to the module parameter,
param_array() updates the sequence elements sequentially:

kernel/params.c:param_array() {
    ...
    *num = 0;
    do {
        ...
        ret = set(val, &kp);
        ...
        (*num)++;
    ...
}

For each element, sysrq_reset_seq_param_set() increments the global
sysrq_reset_seq_version before param_array() increments the global length
sysrq_reset_seq_len (via *num).

If a hardware keypress triggers sysrq_filter() in the window after the
version is incremented but before the length is updated:

1. sysrq_handle_keypress() sees a version mismatch and calls
   sysrq_parse_reset_sequence().
2. The newly added state->reset_seq_len = 0 clears the sequence length.
3. The loop reads the transient truncated sysrq_reset_seq_len.
4. state->reset_seq_version permanently caches the new version.

Once param_array() finishes and corrects sysrq_reset_seq_len, subsequent
keypresses will see matching versions and skip reparsing.

Could this cause a regression by permanently leaving the device with a
truncated reset sequence?

>  	state->reset_seq_cnt = 0;
>  
>  	for (i = 0; i < sysrq_reset_seq_len; i++) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016070636.2559899-1-zhanxusheng@xiaomi.com?part=1