linux-kernel - [PATCH] serial: core: Fix double fetch in uart_throttle/uart

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: <20240112121844.17580-1-2045gemini@gmail.com>
Date: Fri, 12 Jan 2024 20:18:44 +0800
From: Gui-Dong Han <2045gemini@...il.com>
To: gregkh@...uxfoundation.org,
	jirislaby@...nel.org,
	ilpo.jarvinen@...ux.intel.com,
	tony@...mide.com,
	l.sanfilippo@...bus.com,
	john.ogness@...utronix.de,
	tglx@...utronix.de,
	andriy.shevchenko@...ux.intel.com
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] serial: core: Fix double fetch in uart_throttle/uart_unthrottle

In uart_throttle() and uart_unthrottle():
    if (port->status & mask) {
        port->ops->throttle/unthrottle(port);
        mask &= ~port->status;
    }
    // Code segment utilizing the mask value to determine UART behavior

In uart_change_line_settings():
    uart_port_lock_irq(uport);
    // Code segment responsible for updating uport->status
    uart_port_unlock_irq(uport);

In the uart_throttle() and uart_unthrottle() functions, there is a double
fetch issue due to concurrent execution with uart_change_line_settings().
In uart_throttle() and uart_unthrottle(), the check
if (port->status & mask) is made, followed by mask &= ~port->status,
where the relevant bits are cleared. However, port->status may be modified
in uart_change_line_settings(). The current implementation does not ensure
atomicity in the access and modification of port->status and mask. This
can result in mask being updated based on a modified port->status value,
leading to improper UART actions.

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 double fetch, it is suggested to add a uart_port_lock pair
in uart_throttle() and uart_unthrottle(). 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: 391f93f2ec9f ("serial: core: Rework hw-assisted flow control support")
Cc: stable@...r.kernel.org
Signed-off-by: Gui-Dong Han <2045gemini@...il.com>
---
 drivers/tty/serial/serial_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 80085b151b34..9d905fdf2843 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -723,11 +723,13 @@ static void uart_throttle(struct tty_struct *tty)
 		mask |= UPSTAT_AUTOXOFF;
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;
-
+
+	uart_port_lock_irq(port);
 	if (port->status & mask) {
 		port->ops->throttle(port);
 		mask &= ~port->status;
 	}
+	uart_port_unlock_irq(port);

 	if (mask & UPSTAT_AUTORTS)
 		uart_clear_mctrl(port, TIOCM_RTS);
@@ -753,10 +755,12 @@ static void uart_unthrottle(struct tty_struct *tty)
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;

+	uart_port_lock_irq(port);
 	if (port->status & mask) {
 		port->ops->unthrottle(port);
 		mask &= ~port->status;
 	}
+	uart_port_unlock_irq(port);

 	if (mask & UPSTAT_AUTORTS)
 		uart_set_mctrl(port, TIOCM_RTS);
-- 
2.34.1