serial: core: Fix double fetch in uart_throttle/uart_unthrottle

[PATCH] serial: core: Fix double fetch in uart_throttle/uart_unthrottle

Posted by Gui-Dong Han 1 year, 11 months ago

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

Re: [PATCH] serial: core: Fix double fetch in uart_throttle/uart_unthrottle

Posted by Ilpo Järvinen 1 year, 11 months ago

On Fri, 12 Jan 2024, Gui-Dong Han 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);
>  
>  	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);

Hi,

This is very bogus "fix". While change to the local variable gets 
"protected", uart_change_line_settings() can race after unlock and the 
value held in mask is again stale.

If, and it's a big if, this is a real problem, the patch does not fix 
anything! It proves your tool is flawed because it doesn't detect the 
race with uart_change_line_settings() issue still exists after this 
non-fix.

So NAK from me. Please provide a real fix instead if you think there is
a real issue.

Also, don't use vague wording like "leading to improper UART action" but 
describe precisely what goes wrong!

-- 
 i.

Re: [PATCH] serial: core: Fix double fetch in uart_throttle/uart_unthrottle

Posted by Gui-Dong Han 1 year, 11 months ago

Hi

I apologize for any issues and lack of clarity in my previous patch.
In patch v2, I've revised the fix to use local variables instead of
locks and improved the description to clearly explain the harm and
potential for concurrency.

The patch was developed and tested on linux-next, not Linux 5.17. My
reference to 5.17 was due to a project I'm working on, which involves
kernel static analysis and a comparison with earlier studies that
support up to Linux 5.17. Therefore, I initially ran my tool on 5.17
to filter potential bugs that are still unaddressed in the upstream.
Then I worked on linux-next to develop and test the patch. I
understand this might have caused misunderstandings, and I'll aim for
clearer communication in future submissions.

Thanks,
Han

Re: [PATCH] serial: core: Fix double fetch in uart_throttle/uart_unthrottle

Posted by Greg KH 1 year, 11 months ago

On Fri, Jan 12, 2024 at 08:18:44PM +0800, Gui-Dong Han 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.

What would be modifying the status and mask at the same point in time?
Are you sure that it is possible do this?

> 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.

5.17 is VERY old and obsolete, please work against 6.7 at the oldest.
No one can take a patch for 5.17 anymore, you know this :(

thanks,

greg k-h

Re: [PATCH] serial: core: Fix double fetch in uart_throttle/uart_unthrottle

Posted by John Ogness 1 year, 11 months ago

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