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: [email protected]
> Signed-off-by: Gui-Dong Han <[email protected]>
> ---
> 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
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.
From the man page for tty_ioctl:
TIOCPKT
Argument: const int *argp
Enable (when *argp is nonzero) or disable packet mode.
Can be applied to the master side of a pseudoterminal
only (and will return ENOTTY otherwise). In packet
mode, each subsequent read(2) will return a packet that
either contains a single nonzero control byte, or has a
single byte containing zero ('\0') followed by data
written on the slave side of the pseudoterminal. If the
first byte is not TI‐ OCPKT_DATA (0), it is an OR of one
or more of the following bits:
TIOCPKT_FLUSHREAD The read queue for the terminal is flushed.
TIOCPKT_FLUSHWRITE The write queue for the terminal is flushed.
TIOCPKT_STOP Output to the terminal is stopped.
TIOCPKT_START Output to the terminal is restarted.
TIOCPKT_DOSTOP The start and stop characters are ^S/^Q.
TIOCPKT_NOSTOP The start and stop characters are not ^S/^Q.
While packet mode is in use, the presence of control status informa‐
tion to be read from the master side may be detected by a select(2)
for exceptional conditions or a poll(2) for the POLLPRI event.
This mode is used by rlogin(1) and rlogind(8) to implement a remote-
echoed, locally ^S/^Q flow-controlled remote login.
The n_tty_read() function is called by the userspace program on the
master side of the pty pair. This is not, strictly speaking a hot
path; it's not on the interrupt service path of the serial driver, for
example. So it's unliklely that "fixing" this problem is going to
result an measurable performance impact.
It's also the case that not taking the spinlock before checking the
packet mode is not necessarily going to be disastrous. Yes, it might
mean that when the user types ^S, sshd might not stop sending
characters to the client right away, and the status report about the
status of the pty gets delayed by a millisecond or two.
So it's actually *not* a big deal. Now, if you want to make the
argument that it would be nice if these sorts of "false positives" are
suppressed so that it's easier to find real bugs, that's one thing.
But if you're looking at proof that your static checker is actually
fixing Real Bugs (tm), this is probably not the best example.
Cheers,
- Ted