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@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(-)
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