tty: fix atomicity violation in n_tty_read

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.

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")
Cc: stable@vger.kernel.org
Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
---
 drivers/tty/n_tty.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Hi

I apologize for any confusion caused by my reference to Linux 5.17 in
the patch description. I'm currently working on a project involving
kernel static analysis to identify atomicity violations, and part of
this work involves comparison with a previous study that supports 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. I want to
clarify that the patch was developed and tested on linux-next. I
realize now that this may have led to misunderstandings, and I will
ensure clearer communication in future submissions.
My experience with Linux kernel contributions is still growing, and I
acknowledge that my recent submission might have been hasty and lacked
thorough consideration, especially regarding the critical nature of
n_tty_read and the potential impacts of the patch, like performance
concerns. I will take more care in future assessments before
submitting patches and continue to familiarize myself with the rules
and practices of the Linux kernel community.

Thanks,
Han

On Sat, Jan 13, 2024 at 12:59:11AM +0800, Gui-Dong Han wrote:
> 
> I apologize for any confusion caused by my reference to Linux 5.17 in
> the patch description. I'm currently working on a project involving
> kernel static analysis to identify atomicity violations, and part of
> this work involves comparison with a previous study that supports 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. I want to
> clarify that the patch was developed and tested on linux-next. I
> realize now that this may have led to misunderstandings, and I will
> ensure clearer communication in future submissions.
> My experience with Linux kernel contributions is still growing, and I
> acknowledge that my recent submission might have been hasty and lacked
> thorough consideration, especially regarding the critical nature of
> n_tty_read and the potential impacts of the patch, like performance
> concerns. I will take more care in future assessments before
> submitting patches and continue to familiarize myself with the rules
> and practices of the Linux kernel community.

In general, static analysis tools need to be supplemented by an
attempt to understand what the code is trying to do.  This code is
related to the packet mode, which is related to pseudo-tty's --- *not*
the linux serial driver.

On Thu, Feb 01, 2024 at 10:02:46AM +0100, Jiri Slaby wrote:
> Hi,
> 
> all your mail is classified as Spam in both my private and company MTAs. Not
> sure what CCs' MTA say as I don't know why (as dkim, spf, dmarc all pass).
> You likely need to fix your mail setup somehow.

It was classified by spam for me too :(