Re: [PATCH] tty: fix atomicity violation in n_tty_read

[Greg KH]

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

Again, we can't do anything with 5.17 patches :(

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

That is not where this code came from :(

> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gui-Dong Han <2045gemini@xxxxxxxxx>
> ---
>  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);

What is this lock going to do for the performance?  The n_tty_read path
is VERY tricky, and heavily used and tested, without a real reproducer
or proof of a bug here, we are going to be very loath to change anything
for obvious reasons.

Also, how was this tested?

thanks,

greg k-h