[PATCH v3 0/2] tty: sysrq: fix the reset sequence state kept per input device

[PATCH v3 0/2] tty: sysrq: fix the reset sequence state kept per input device

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

Sashiko's review of v2 asked whether clearing reset_keybit could leave a
device with a permanently truncated reset sequence.  It can, and the
answer turned out to matter more than I assumed when I first looked at it:
with the default sysrq_downtime_ms of 0 the truncated sequence reboots the
machine on a single keypress.

That outcome is already reachable without the v2 change, on a device that
has not parsed a sequence yet and therefore starts with an empty
reset_keybit.  But v2 extends it to every device, because clearing the
bitmap turns what used to be a stale superset of the old and new keys into
a proper truncation.  So the race is fixed first, in 1/2, and 2/2 is the
v2 patch unchanged.

Changes in v3:
 - Added 1/2 so that a truncated parse no longer sticks (Sashiko).
 - Added Fixes: to both patches.

Changes in v2:
 - Retitled from "Input:" to "tty: sysrq:" (Greg Kroah-Hartman).
 - Delete keyreset_timer, otherwise an already armed reset survives the
   reparse (Sashiko).
 - Added Cc: stable.

Zhan Xusheng (2):
  tty: sysrq: reparse the reset sequence when its length changes
  tty: sysrq: reset the per-device key state when reparsing reset_seq

 drivers/tty/sysrq.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)


base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0

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

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

reset_seq is a module parameter array, so a write to it is handled by
param_array(), which publishes the new length only after the element
setter has already bumped the version:

	kernel/params.c:
		*num = 0;			/* sysrq_reset_seq_len */
		do {
			ret = set(val, &kp);	/* bumps the version */
			...
			(*num)++;
		} while (save == ',');

sysrq_handle_keypress() decides to reparse purely on the version, so a
keypress landing between the last setter and the matching (*num)++ parses
a sequence that is one element short and then latches the final version.
Nothing makes it reparse afterwards, so the device keeps the truncated
sequence until the parameter is written again.

On a device that has not parsed before, a two-key sequence truncated this
way leaves reset_seq_len at 1.  The parse runs before the keypress is
recorded in key_down, so reset_seq_cnt is 0 and reset_canceled stays
false.  A single press of the remaining key then satisfies

	if (++state->reset_seq_cnt == state->reset_seq_len &&
	    !state->reset_canceled)

and sysrq_handle_reset_request() goes straight to orderly_reboot(),
because sysrq_downtime_ms defaults to 0.

Record the length the parse used and treat a change in it as staleness
too.  The truncated parse then no longer sticks: once param_array() has
published the final length, the next keypress reparses.

Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-173915@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 drivers/tty/sysrq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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;
 }
 
 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);
 
 		/*
-- 
2.43.0

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

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

sysrq_parse_reset_sequence() clears reset_seq_cnt, but reset_keybit is
only ever __set_bit() and reset_seq_len only ever incremented.  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 mode 0644 reset_seq
parameter is written.

After writing 56,29,111 and then 59,60 a device keeps all five keys in
reset_keybit with reset_seq_len 5, so the effective combination is the
union of the old and the new sequence.  Holding just 59 and 60 counts two
of five and does nothing, while releasing a stale key drives
reset_seq_cnt below zero.

Clear both along with reset_seq_cnt.  Also delete keyreset_timer: with
sysrq_downtime_ms set, holding a full sequence arms it, and it is only
deleted on a press outside the sequence or a release inside it.  Dropping
the old keys from reset_keybit removes both of those cancellations, so an
already armed reset would survive into orderly_reboot().

Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
Cc: stable@vger.kernel.org
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 drivers/tty/sysrq.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 4fd5b3ace9c40..bb202fc1ccf0b 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -691,6 +691,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 < len; i++) {
-- 
2.43.0