Re: [PATCH v3 1/2] tty: sysrq: reparse the reset sequence when its length changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: sysrq: incomplete race fix in sysrq_handle_keypress() during param_array()
- [Medium] tty: sysrq: unsynchronized version latching in sysrq_parse_reset_sequence()
--

commit 969b9cd1cf7c6541203279c0b287db9c5b090a8b
Author: Zhan Xusheng <zhanxusheng@xiaomi.com>

tty: sysrq: reparse the reset sequence when its length changes

This commit tracks the parsed reset sequence length in struct sysrq_state and
triggers a reparse in sysrq_handle_keypress() when sysrq_reset_seq_len no
longer matches the previously parsed length.

> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index c2e4b31b699aa..4fd5b3ace9c40 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -676,6 +676,7 @@ struct sysrq_state {
>  	int reset_seq_len;
>  	int reset_seq_cnt;
>  	int reset_seq_version;
> +	unsigned int reset_seq_parsed_len;
>  	struct timer_list keyreset_timer;
>  };
>  
> @@ -686,12 +687,13 @@ static unsigned int sysrq_reset_seq_version = 1;
>  
>  static void sysrq_parse_reset_sequence(struct sysrq_state *state)
>  {
> +	unsigned int len = READ_ONCE(sysrq_reset_seq_len);
>  	int i;
>  	unsigned short key;
>  
>  	state->reset_seq_cnt = 0;
>  
> -	for (i = 0; i < sysrq_reset_seq_len; i++) {
> +	for (i = 0; i < len; i++) {
>  		key = sysrq_reset_seq[i];
>  
>  		if (key == KEY_RESERVED || key > KEY_MAX)
> @@ -708,6 +710,7 @@ static void sysrq_parse_reset_sequence(struct sysrq_state *state)
>  	state->reset_canceled = state->reset_seq_cnt != 0;
>  
>  	state->reset_seq_version = sysrq_reset_seq_version;
> +	state->reset_seq_parsed_len = len;
>  }

[Severity: Medium]
Could sysrq_parse_reset_sequence() still latch a stale or partially updated
key sequence when racing with param_array() -> sysrq_reset_seq_param_set()?

In sysrq_parse_reset_sequence(), len is read from sysrq_reset_seq_len at
the start of the function and sysrq_reset_seq[i] is read in the loop, while
sysrq_reset_seq_version is read and latched into state->reset_seq_version at
the end without verifying whether sysrq_reset_seq_version changed in the
meantime (and the follow-up commit "tty: sysrq: reset the per-device key
state when reparsing reset_seq" adds timer_delete() and bitmap_zero() between
reading len and latching sysrq_reset_seq_version, widening this window):

drivers/tty/sysrq.c:sysrq_parse_reset_sequence() {
	unsigned int len = READ_ONCE(sysrq_reset_seq_len);
	...
	for (i = 0; i < len; i++) {
		key = sysrq_reset_seq[i];
		...
	}
	...
	state->reset_seq_version = sysrq_reset_seq_version;
	state->reset_seq_parsed_len = len;
}

If a concurrent write to /sys/module/sysrq/parameters/reset_seq updates the
sequence to the same final length and increments sysrq_reset_seq_version
before state->reset_seq_version is assigned, state->reset_seq_version and
state->reset_seq_parsed_len will both match the final published values even
though stale or mixed sysrq_reset_seq[] elements were parsed.

In addition, sysrq_reset_seq_param_set() and param_array() update
sysrq_reset_seq[i], sysrq_reset_seq_version, and sysrq_reset_seq_len without
memory barriers, and sysrq_parse_reset_sequence() reads them without read
barriers:

drivers/tty/sysrq.c:sysrq_reset_seq_param_set() {
	...
	*((unsigned short *)kp->arg) = val;
	sysrq_reset_seq_version++;
}

On weakly-ordered architectures, can sysrq_parse_reset_sequence() observe the
updated sysrq_reset_seq_len and sysrq_reset_seq_version before the stores to
sysrq_reset_seq[] are visible, permanently latching stale keys?

>  static void sysrq_do_reset(struct timer_list *t)
> @@ -903,7 +906,9 @@ static bool sysrq_handle_keypress(struct sysrq_state *sysrq,
>  		/*
>  		 * See if reset sequence has changed since the last time.
>  		 */
> -		if (sysrq->reset_seq_version != sysrq_reset_seq_version)
> +		if (sysrq->reset_seq_version != sysrq_reset_seq_version ||
> +		    sysrq->reset_seq_parsed_len !=
> +				READ_ONCE(sysrq_reset_seq_len))
>  			sysrq_parse_reset_sequence(sysrq);
>  
>  		/*

[Severity: High]
What happens if the first key of a new multi-key sequence is pressed while
param_array() is still parsing the remaining elements?

When writing a multi-key sequence to /sys/module/sysrq/parameters/reset_seq,
param_array() resets *num (sysrq_reset_seq_len) to 0 and increments it after
each element setter call:

kernel/params.c:param_array() {
	*num = 0;
	do {
		...
		ret = set(val, &kp);
		if (ret != 0)
			return ret;
		kp.arg += elemsize;
		val += len+1;
		(*num)++;
	} while (save == ',');
}

After the first key is stored in sysrq_reset_seq[0] and (*num)++ sets
sysrq_reset_seq_len to 1, param_array() proceeds to parse the second element.
If a press of that first key (value == 1) arrives in sysrq_handle_keypress()
during this window, sysrq_parse_reset_sequence() reads len == 1 and configures
a 1-key reset sequence with state->reset_seq_len == 1, state->reset_seq_cnt ==
0, and state->reset_canceled == false.

Immediately afterwards in the same sysrq_handle_keypress() call,
sysrq_detect_reset_sequence() runs before any subsequent keypress can reparse
the final length:

drivers/tty/sysrq.c:sysrq_handle_keypress() {
		if (sysrq->reset_seq_version != sysrq_reset_seq_version ||
		    sysrq->reset_seq_parsed_len !=
				READ_ONCE(sysrq_reset_seq_len))
			sysrq_parse_reset_sequence(sysrq);

		if (value)
			set_bit(code, sysrq->key_down);
		else
			clear_bit(code, sysrq->key_down);

		if (was_active)
			schedule_work(&sysrq->reinject_work);

		/* Check for reset sequence */
		sysrq_detect_reset_sequence(sysrq, code, value);
}

In sysrq_detect_reset_sequence(), ++state->reset_seq_cnt ==
state->reset_seq_len (1 == 1) is satisfied with !state->reset_canceled, which
calls sysrq_handle_reset_request() -> sysrq_do_reset() -> orderly_reboot()
immediately on that single keypress.

Also, because param_array() sets *num = 0 before calling the setter for the
first element, checking sysrq->reset_seq_parsed_len !=
READ_ONCE(sysrq_reset_seq_len) causes a concurrent keypress to reparse an
empty sequence (len == 0) before the first setter even runs (which, with the
follow-up commit "tty: sysrq: reset the per-device key state when reparsing
reset_seq", also clears reset_keybit and deletes keyreset_timer even if the
parameter write fails on the first element).

Would it be safer to synchronize updates to sysrq_reset_seq[] and
sysrq_reset_seq_len so that sysrq_handle_keypress() never observes intermediate
states while param_array() is in progress?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021022007.1717207-1-zhanxusheng1024@gmail.com?part=1