linux-kernel - [PATCH] tty: fix atomicity violation in n_tty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240112125801.2650-1-2045gemini@gmail.com>
Date: Fri, 12 Jan 2024 20:58:01 +0800
From: Gui-Dong Han <2045gemini@...il.com>
To: gregkh@...uxfoundation.org,
	jirislaby@...nel.org
Cc: linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org,
	baijiaju1990@...look.com,
	Gui-Dong Han <2045gemini@...il.com>,
	stable@...r.kernel.org
Subject: [PATCH] tty: fix atomicity violation in n_tty_read

In n_tty_read():
    if (packet && tty->link->ctrl.pktstatus) {
    ...
    spin_lock_irq(&tty->link->ctrl.lock);
    cs = tty->link->ctrl.pktstatus;
    tty->link->ctrl.pktstatus = 0;
    spin_unlock_irq(&tty->link->ctrl.lock);
    *kb++ = cs;
    ...

In n_tty_read() function, there is a potential atomicity violation issue.
The tty->link->ctrl.pktstatus might be set to 0 after being checked, which
could lead to incorrect values in the kernel space buffer
pointer (kb/kbuf). The check if (packet && tty->link->ctrl.pktstatus)
occurs outside the spin_lock_irq(&tty->link->ctrl.lock) block. This may
lead to tty->link->ctrl.pktstatus being altered between the check and the
lock, causing *kb++ = cs; to be assigned with a zero pktstatus value.

This possible bug is found by an experimental static analysis tool
developed by our team, BassCheck[1]. This tool analyzes the locking APIs
to extract function pairs that can be concurrently executed, and then
analyzes the instructions in the paired functions to identify possible
concurrency bugs including data races and atomicity violations. The above
possible bug is reported when our tool analyzes the source code of
Linux 5.17.

To resolve this atomicity issue, it is suggested to move the condition
check if (packet && tty->link->ctrl.pktstatus) inside the spin_lock block.
With this patch applied, our tool no longer reports the bug, with the
kernel configuration allyesconfig for x86_64. Due to the absence of the
requisite hardware, we are unable to conduct runtime testing of the patch.
Therefore, our verification is solely based on code logic analysis.

[1] https://sites.google.com/view/basscheck/

Fixes: 64d608db38ff ("tty: cumulate and document tty_struct::ctrl* members")
Cc: stable@...r.kernel.org
Signed-off-by: Gui-Dong Han <2045gemini@...il.com>
---
 drivers/tty/n_tty.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index f252d0b5a434..df54ab0c4d8c 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2222,19 +2222,23 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, u8 *kbuf,
 	add_wait_queue(&tty->read_wait, &wait);
 	while (nr) {
 		/* First test for status change. */
+		spin_lock_irq(&tty->link->ctrl.lock);
 		if (packet && tty->link->ctrl.pktstatus) {
 			u8 cs;
-			if (kb != kbuf)
+			if (kb != kbuf) {
+				spin_unlock_irq(&tty->link->ctrl.lock);
 				break;
-			spin_lock_irq(&tty->link->ctrl.lock);
+			}
 			cs = tty->link->ctrl.pktstatus;
 			tty->link->ctrl.pktstatus = 0;
 			spin_unlock_irq(&tty->link->ctrl.lock);
 			*kb++ = cs;
 			nr--;
 			break;
+		} else {
+			spin_unlock_irq(&tty->link->ctrl.lock);
 		}
-
+
 		if (!input_available_p(tty, 0)) {
 			up_read(&tty->termios_rwsem);
 			tty_buffer_flush_work(tty->port);
-- 
2.34.1