Received-SPF: pass (google.com: domain of linux-kernel+bounces-24691-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: John Ogness <john.ogness@linutronix.de>
To: Gui-Dong Han <2045gemini@gmail.com>, gregkh@linuxfoundation.org,
 jirislaby@kernel.org, ilpo.jarvinen@linux.intel.com, tony@atomide.com,
 l.sanfilippo@kunbus.com, tglx@linutronix.de,
 andriy.shevchenko@linux.intel.com
Cc: linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
 baijiaju1990@outlook.com, Gui-Dong Han <2045gemini@gmail.com>,
 stable@vger.kernel.org
Subject: Re: [PATCH] serial: core: Fix double fetch in
 uart_throttle/uart_unthrottle
In-Reply-To: <20240112121844.17580-1-2045gemini@gmail.com>
References: <20240112121844.17580-1-2045gemini@gmail.com>
Date: Fri, 12 Jan 2024 14:34:52 +0106
Message-ID: <87h6ji7ka3.fsf@jogness.linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 2024-01-12, Gui-Dong Han <2045gemini@gmail.com> wrote:
> 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@vger.kernel.org
> Signed-off-by: Gui-Dong Han <2045gemini@gmail.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);

You would also need to remove uart_port_lock_irq() out of all the
throttle() callbacks.

>  
>  	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);

You would also need to remove uart_port_lock_irq() out of all the
unthrottle() callbacks.

John Ogness