[PATCH] serial: core: Fix atomicity violation in uart_tiocmget

[PATCH] serial: core: Fix atomicity violation in uart_tiocmget

[Gui-Dong Han]

In uart_tiocmget():
    result = uport->mctrl;
    uart_port_lock_irq(uport);
    result |= uport->ops->get_mctrl(uport);
    uart_port_unlock_irq(uport);
    ...
    return result;

In uart_update_mctrl():
    uart_port_lock_irqsave(port, &flags);
    ...
    port->mctrl = (old & ~clear) | set;
    ...
    uart_port_unlock_irqrestore(port, flags);

An atomicity violation is identified due to the concurrent execution of
uart_tiocmget() and uart_update_mctrl(). After assigning
result = uport->mctrl, the mctrl value may change in uart_update_mctrl(),
leading to a mismatch between the value returned by
uport->ops->get_mctrl(uport) and the mctrl value previously read.
This can result in uart_tiocmget() returning an incorrect 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 address this issue, it is suggested to move the line
result = uport->mctrl inside the uart_port_lock block to ensure atomicity
and prevent the mctrl value from being altered during the execution of
uart_tiocmget(). 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: 559c7ff4e324 ("serial: core: Use port lock wrappers")
Cc: stable@vger.kernel.org
Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
---
 drivers/tty/serial/serial_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 80085b151b34..a9e39416d877 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1085,8 +1085,8 @@ static int uart_tiocmget(struct tty_struct *tty)
 		goto out;
 
 	if (!tty_io_error(tty)) {
-		result = uport->mctrl;
 		uart_port_lock_irq(uport);
+		result = uport->mctrl;
 		result |= uport->ops->get_mctrl(uport);
 		uart_port_unlock_irq(uport);
 	}
-- 
2.34.1

Re: [PATCH] serial: core: Fix atomicity violation in uart_tiocmget

[John Ogness]

On 2024-01-12, Gui-Dong Han <2045gemini@gmail.com> wrote:
> In uart_tiocmget():
>     result = uport->mctrl;
>     uart_port_lock_irq(uport);
>     result |= uport->ops->get_mctrl(uport);
>     uart_port_unlock_irq(uport);
>     ...
>     return result;
>
> In uart_update_mctrl():
>     uart_port_lock_irqsave(port, &flags);
>     ...
>     port->mctrl = (old & ~clear) | set;
>     ...
>     uart_port_unlock_irqrestore(port, flags);
>
> An atomicity violation is identified due to the concurrent execution of
> uart_tiocmget() and uart_update_mctrl(). After assigning
> result = uport->mctrl, the mctrl value may change in uart_update_mctrl(),
> leading to a mismatch between the value returned by
> uport->ops->get_mctrl(uport) and the mctrl value previously read.
> This can result in uart_tiocmget() returning an incorrect 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 address this issue, it is suggested to move the line
> result = uport->mctrl inside the uart_port_lock block to ensure atomicity
> and prevent the mctrl value from being altered during the execution of
> uart_tiocmget(). 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: 559c7ff4e324 ("serial: core: Use port lock wrappers")

It fixes c5f4644e6c8b ("[PATCH] Serial: Adjust serial locking").

> Cc: stable@vger.kernel.org
> Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
> ---
>  drivers/tty/serial/serial_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 80085b151b34..a9e39416d877 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1085,8 +1085,8 @@ static int uart_tiocmget(struct tty_struct *tty)
>  		goto out;
>  
>  	if (!tty_io_error(tty)) {
> -		result = uport->mctrl;
>  		uart_port_lock_irq(uport);
> +		result = uport->mctrl;
>  		result |= uport->ops->get_mctrl(uport);
>  		uart_port_unlock_irq(uport);
>  	}

Looking over the RMW accesses to @mctrl, I expect you will also need
this hunk:

@@ -2242,6 +2242,7 @@ uart_set_options(struct uart_port *port, struct console *co,
 {
 	struct ktermios termios;
 	static struct ktermios dummy;
+	unsigned long flags;
 
 	/*
 	 * Ensure that the serial-console lock is initialised early.
@@ -2279,7 +2280,9 @@ uart_set_options(struct uart_port *port, struct console *co,
 	 * some uarts on other side don't support no flow control.
 	 * So we set * DTR in host uart to make them happy
 	 */
+	uart_port_lock_irqsave(port, &flags);
 	port->mctrl |= TIOCM_DTR;
+	uart_port_unlock_irqrestore(port, flags);
 
 	port->ops->set_termios(port, &termios, &dummy);
 	/*

FWIW,
Reviewed-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH] serial: core: Fix atomicity violation in uart_tiocmget

[Ilpo Järvinen]

On Fri, 12 Jan 2024, John Ogness wrote:

> On 2024-01-12, Gui-Dong Han <2045gemini@gmail.com> wrote:
> > In uart_tiocmget():
> >     result = uport->mctrl;
> >     uart_port_lock_irq(uport);
> >     result |= uport->ops->get_mctrl(uport);
> >     uart_port_unlock_irq(uport);
> >     ...
> >     return result;
> >
> > In uart_update_mctrl():
> >     uart_port_lock_irqsave(port, &flags);
> >     ...
> >     port->mctrl = (old & ~clear) | set;
> >     ...
> >     uart_port_unlock_irqrestore(port, flags);
> >
> > An atomicity violation is identified due to the concurrent execution of
> > uart_tiocmget() and uart_update_mctrl(). After assigning
> > result = uport->mctrl, the mctrl value may change in uart_update_mctrl(),
> > leading to a mismatch between the value returned by
> > uport->ops->get_mctrl(uport) and the mctrl value previously read.
> > This can result in uart_tiocmget() returning an incorrect 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 address this issue, it is suggested to move the line
> > result = uport->mctrl inside the uart_port_lock block to ensure atomicity
> > and prevent the mctrl value from being altered during the execution of
> > uart_tiocmget(). 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: 559c7ff4e324 ("serial: core: Use port lock wrappers")
> 
> It fixes c5f4644e6c8b ("[PATCH] Serial: Adjust serial locking").

That commit only extracted the locks from ->get_mctrl() into the caller
but this assignment was outside both pre and post that commit (the issue 
goes all the way back into history.git domain into 33c0d1b0c3eb ("[PATCH] 
Serial driver stuff") which introduced ->mctrl).

-- 
 i.

> > Cc: stable@vger.kernel.org
> > Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
> > ---
> >  drivers/tty/serial/serial_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 80085b151b34..a9e39416d877 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1085,8 +1085,8 @@ static int uart_tiocmget(struct tty_struct *tty)
> >  		goto out;
> >  
> >  	if (!tty_io_error(tty)) {
> > -		result = uport->mctrl;
> >  		uart_port_lock_irq(uport);
> > +		result = uport->mctrl;
> >  		result |= uport->ops->get_mctrl(uport);
> >  		uart_port_unlock_irq(uport);
> >  	}
> 
> Looking over the RMW accesses to @mctrl, I expect you will also need
> this hunk:
> 
> @@ -2242,6 +2242,7 @@ uart_set_options(struct uart_port *port, struct console *co,
>  {
>  	struct ktermios termios;
>  	static struct ktermios dummy;
> +	unsigned long flags;
>  
>  	/*
>  	 * Ensure that the serial-console lock is initialised early.
> @@ -2279,7 +2280,9 @@ uart_set_options(struct uart_port *port, struct console *co,
>  	 * some uarts on other side don't support no flow control.
>  	 * So we set * DTR in host uart to make them happy
>  	 */
> +	uart_port_lock_irqsave(port, &flags);
>  	port->mctrl |= TIOCM_DTR;
> +	uart_port_unlock_irqrestore(port, flags);
>  
>  	port->ops->set_termios(port, &termios, &dummy);
>  	/*
> 
> FWIW,
> Reviewed-by: John Ogness <john.ogness@linutronix.de>
> 

Re: [PATCH] serial: core: Fix atomicity violation in uart_tiocmget

[Gui-Dong Han]

Hi,

You are correct about the 'Fixes' tag. It should indeed be c5f4644e6c8b 
("[PATCH] Serial: Adjust serial locking"). I will update this in the 
patch v2.


Regarding the issue found in Linux 5.17, I mistakenly used git blame 
which led to the incorrect identification of commit 559c7ff4e324. The 
issue indeed exists in Linux 5.17 and I acknowledge the error in tracing 
the commit.

In uart_tiocmget(), the result variable is stable. However, there's a 
risk of inconsistency due to the updates in uart_update_mctrl(). 
Consider a scenario where uart_tiocmget() reads uport->mctrl into result 
before entering the critical section. If uart_update_mctrl() updates 
port->mctrl and calls set_mctrl concurrently, the subsequent execution 
of result |= uport->ops->get_mctrl(uport); in uart_tiocmget() might 
yield an inaccurate result. This happens because result contains the old 
value of port->mctrl, which no longer matches the updated state 
retrieved by get_mctrl.

Thanks,

Han

Re: [PATCH] serial: core: Fix atomicity violation in uart_tiocmget

[Ilpo Järvinen]

On Fri, 12 Jan 2024, Gui-Dong Han wrote:

> In uart_tiocmget():
>     result = uport->mctrl;
>     uart_port_lock_irq(uport);
>     result |= uport->ops->get_mctrl(uport);
>     uart_port_unlock_irq(uport);
>     ...
>     return result;
> 
> In uart_update_mctrl():
>     uart_port_lock_irqsave(port, &flags);
>     ...
>     port->mctrl = (old & ~clear) | set;
>     ...
>     uart_port_unlock_irqrestore(port, flags);
> 
> An atomicity violation is identified due to the concurrent execution of
> uart_tiocmget() and uart_update_mctrl(). After assigning
> result = uport->mctrl, the mctrl value may change in uart_update_mctrl(),
> leading to a mismatch between the value returned by
> uport->ops->get_mctrl(uport) and the mctrl value previously read.
> This can result in uart_tiocmget() returning an incorrect 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 address this issue, it is suggested to move the line
> result = uport->mctrl inside the uart_port_lock block to ensure atomicity
> and prevent the mctrl value from being altered during the execution of
> uart_tiocmget(). 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: 559c7ff4e324 ("serial: core: Use port lock wrappers")

This is clearly incorrect, also pre-559c7ff4e324 kernels have the 
assignment outside the critical section.

It's also non-sensical given that 559c7ff4e324 is quite recent. You 
claim to have found the issue in Linux 5.17. How come can you seriously 
claim that 559c7ff4e324 that isn't even present in Linux 5.17 is the 
commit that needs to be fixed?

> Cc: stable@vger.kernel.org
> Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
> ---
>  drivers/tty/serial/serial_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 80085b151b34..a9e39416d877 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1085,8 +1085,8 @@ static int uart_tiocmget(struct tty_struct *tty)
>  		goto out;
>  
>  	if (!tty_io_error(tty)) {
> -		result = uport->mctrl;
>  		uart_port_lock_irq(uport);
> +		result = uport->mctrl;
>  		result |= uport->ops->get_mctrl(uport);
>  		uart_port_unlock_irq(uport);
>  	}
> 

The change itself looks quite harmless but it provides no quarantees that 
the result is up-to-date after uart_port_unlock_irq(), so while it solves 
the data race on paper it cannot fundamentally prevent racing after 
unlock. And again "result" variable containing stale information.

-- 
 i.