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

[sashiko-bot@kernel.org]

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