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
