drivers/tty/n_tty.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
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(-)
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);
if (packet && tty->link->ctrl.pktstatus) {
u8 cs;
- if (kb != kbuf)
+ if (kb != kbuf) {
+ spin_unlock_irq(&tty->link->ctrl.lock);
break;
- 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;
nr--;
break;
+ } else {
+ spin_unlock_irq(&tty->link->ctrl.lock);
}
-
+
if (!input_available_p(tty, 0)) {
up_read(&tty->termios_rwsem);
tty_buffer_flush_work(tty->port);
--
2.34.1Hi, 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. Headers: > Delivered-To: jirislaby+korg@gmail.com > Received: by 2002:ac0:d5cb:0:b0:2fd:5d7f:bc30 with SMTP id u11csp768930imh; > Fri, 12 Jan 2024 04:58:18 -0800 (PST) > X-Google-Smtp-Source: AGHT+IF/3LYNmNd1d7yS4c746ryIOjwWPOlNU7fcqvZ7ycEWFsrcPzEYgEqRc5d2BrPcf8yom1He > X-Received: by 2002:a17:906:29ce:b0:a28:b085:5a86 with SMTP id y14-20020a17090629ce00b00a28b0855a86mr594229eje.133.1705064298179; > Fri, 12 Jan 2024 04:58:18 -0800 (PST) > ARC-Seal: i=1; a=rsa-sha256; t=1705064298; cv=none; > d=google.com; s=arc-20160816; > b=X5hO2nwthigWdqlDEp9mlArxmch62iPLf7BohvIaT+FLSQtkY/wROZtenuCZT2bV9y > NhTy4ua83SjlXUsReLP7dwbOjQilwdqiKFPMKTn+tJmShfcxdaYjvpAr7apSZi4YpEQf > uRoWK7W8JfpVKFQT4D8cHv9+hliBxhmC4GRLdKtp564+bpO3j/4zsENIe9saFzMNqmM8 > QVhZXotHRShB2FdZc/VmxiNe4eXnxJ8KIhQMJcuo3LdXRiWrQ/2vXgBcIfsfr4FQeZM3 > xacjxp9OrrdUlUZQdY1JmfiU13iN+b4oiu7W2suuj2zSUbdUT3vcufTBDchwEX7S84Oi > Sy7w== > ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; > h=content-transfer-encoding:mime-version:message-id:date:subject:cc > :to:from:dkim-signature:dmarc-filter:delivered-to; > bh=SKCP4kSmi8Znay+ncNnEwwM69gACCAM8H97RoLluFSU=; > fh=fP+uh1DN39DuEzOCSiaLKB3WTgSPWFk9ij8kCOsTwec=; > b=acW4d8Z6n1mvqA9PwE3vDYYyHpDkDTNIfnmf9bbIVDFRzIZ1g5eGCnxCmJ9kA68nAW > AZ79DUtrCYuuWsW5mLidpsdDDKWB5iVy7xpTtlTOhrA1A3QHk7d9afk7d31lu73SLJ+s > cim0UxXCsyOIdPeWlaKeKB3u2gozSBP/wN/jMtqxfzbvZxjZV451c3eSSI/093s5Nj3/ > U8tjiUDidbRkRIk2GRiwOMU2FIEORIyAylSNBtGJvlQ1CNavVQ3nU/OcMeaQ150+FIKD > gQY4khoMEyVPph27tYwZh6exWeZcuw2jVG72JkTJs3ZEwR/B+bPBobFPjIK4UzkSwn98 > yTxA== > ARC-Authentication-Results: i=1; mx.google.com; > dkim=pass header.i=@gmail.com header.s=20230601 header.b=EvmWOavP; > spf=pass (google.com: domain of srs0=twir=iw=gmail.com=2045gemini@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom="SRS0=Twir=IW=gmail.com=2045gemini@kernel.org"; > dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com > Return-Path: <SRS0=Twir=IW=gmail.com=2045gemini@kernel.org> > Received: from ams.source.kernel.org (ams.source.kernel.org. [145.40.68.75]) > by mx.google.com with ESMTPS id t10-20020a1709063e4a00b00a2a224d5420si1409203eji.776.2024.01.12.04.58.18 > for <jirislaby+korg@gmail.com> > (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); > Fri, 12 Jan 2024 04:58:18 -0800 (PST) > Received-SPF: pass (google.com: domain of srs0=twir=iw=gmail.com=2045gemini@kernel.org designates 145.40.68.75 as permitted sender) client-ip=145.40.68.75; > Authentication-Results: mx.google.com; > dkim=pass header.i=@gmail.com header.s=20230601 header.b=EvmWOavP; > spf=pass (google.com: domain of srs0=twir=iw=gmail.com=2045gemini@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom="SRS0=Twir=IW=gmail.com=2045gemini@kernel.org"; > dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com > Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) > by ams.source.kernel.org (Postfix) with ESMTP id C5199B82253 > for <jirislaby+korg@gmail.com>; Fri, 12 Jan 2024 12:58:17 +0000 (UTC) > Received: by smtp.kernel.org (Postfix) > id 3B5B1C43399; Fri, 12 Jan 2024 12:58:17 +0000 (UTC) > Delivered-To: jirislaby@kernel.org > Received: from mail-oo1-f48.google.com (mail-oo1-f48.google.com [209.85.161.48]) > (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) > key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) > (No client certificate requested) > by smtp.kernel.org (Postfix) with ESMTPS id 0BCAEC433B1 > for <jirislaby@kernel.org>; Fri, 12 Jan 2024 12:58:15 +0000 (UTC) > DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.kernel.org 0BCAEC433B1 > Authentication-Results: smtp.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com > Authentication-Results: smtp.kernel.org; spf=pass smtp.mailfrom=gmail.com > Received: by mail-oo1-f48.google.com with SMTP id 006d021491bc7-598c92465c2so263240eaf.3 > for <jirislaby@kernel.org>; Fri, 12 Jan 2024 04:58:15 -0800 (PST) > DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; > d=gmail.com; s=20230601; t=1705064295; x=1705669095; darn=kernel.org; > h=content-transfer-encoding:mime-version:message-id:date:subject:cc > :to:from:from:to:cc:subject:date:message-id:reply-to; > bh=SKCP4kSmi8Znay+ncNnEwwM69gACCAM8H97RoLluFSU=; > b=EvmWOavPxwjgc+yDYIiyY/w/z1xhXMRSVpdywGaxL7rklmdTONNRCxvtacNNITc+5J > kw7sEpuBdUYGg9ynslYzKdiJqEaMqG/tVw1VA8WglLrc5WDiWVe0Ly6pAF0n7qtVmPVi > pieUNXbpLlfcVuDrqczwe9IdUuAL+d+UNkqf+GkHSVJ/NzKicOB160AvopFIYN6Qoj/d > qCQVQ4iy3p44eZ86MbztWy94mN/JDaXJjFbvuI5v/Cfx8LmJJT0V44VBmD3mPPGNAzy0 > Ho/htuIS78Mz8hEf+JnoHkVsHH+l3R9XixAlz4L3w+eM1LxwNzK1AXbGZnAdBsLmbAIr > 4YgQ== > X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; > d=1e100.net; s=20230601; t=1705064295; x=1705669095; > h=content-transfer-encoding:mime-version:message-id:date:subject:cc > :to:from:x-gm-message-state:from:to:cc:subject:date:message-id > :reply-to; > bh=SKCP4kSmi8Znay+ncNnEwwM69gACCAM8H97RoLluFSU=; > b=Rkanvc4aSfPbP0ynJz41MBjbAVn0zKy8x8W+ftdoY8DkbJ5x7/ENKVUjLgRkbXWuQH > bTp5BX8j7EF0EwnZ1ut2KYQPO1Nrb29If9XSMcG8D2fhaKSraPV7gwgVLwTmY6J2ObwV > EOv3uJXr4BVngTXEMdCG1uGhoW70YBDFciT2iHbK2aZ6oz3CdVAdQznVlxJXArRmzqcv > GvQvrlWI+qt7QWKiXzqI4DRkzEuyK/qNLfu67NyJ1qssm9mNt0il/GYTZkqdfuI/wxlI > yWtl4b0qyaEmNFVOIF8B8BLSJWs3y5YH+WZQxQSfZGrg26cN0OlBtj3N4oOKAIaOCH0Y > WI9Q== > X-Gm-Message-State: AOJu0Yyyn81BwhbCgzcpJ5tAP9T/AjxpT0em8lzR+hmuGwzP3IAEgJw+ > +sAIT5qrT4keqaUNgAJRtBU= > X-Received: by 2002:a05:6358:4d83:b0:172:ae2a:2256 with SMTP id cc3-20020a0563584d8300b00172ae2a2256mr1350395rwb.27.1705064294552; > Fri, 12 Jan 2024 04:58:14 -0800 (PST) > Received: from g2039B650.. ([106.39.42.152]) > by smtp.gmail.com with ESMTPSA id fd14-20020a056a002e8e00b006dad4c91e8fsm3103080pfb.205.2024.01.12.04.58.11 > (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); > Fri, 12 Jan 2024 04:58:14 -0800 (PST) > From: Gui-Dong Han <2045gemini@gmail.com> > To: gregkh@linuxfoundation.org, > jirislaby@kernel.org > Cc: linux-kernel@vger.kernel.org, > linux-serial@vger.kernel.org, > baijiaju1990@outlook.com, > Gui-Dong Han <2045gemini@gmail.com>, > stable@vger.kernel.org > Subject: [PATCH] tty: fix atomicity violation in n_tty_read > Date: Fri, 12 Jan 2024 20:58:01 +0800 > Message-Id: <20240112125801.2650-1-2045gemini@gmail.com> > X-Mailer: git-send-email 2.34.1 > MIME-Version: 1.0 > Content-Transfer-Encoding: 8bit -- js suse labs
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 :(
On Fri, 12 Jan 2024, 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.
>
> 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(-)
>
> 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);
> if (packet && tty->link->ctrl.pktstatus) {
> u8 cs;
> - if (kb != kbuf)
> + if (kb != kbuf) {
> + spin_unlock_irq(&tty->link->ctrl.lock);
> break;
> - 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;
> nr--;
> break;
> + } else {
> + spin_unlock_irq(&tty->link->ctrl.lock);
This seems way over-engineered. Wouldn't it be much simpler to just test
if cs is non-zero after the original critical section to detect if there
were any changes into pktstatus before the lock was acquired? That would
avoid all this lock dance and enlarging the critical section.
> }
> -
> +
Spurious changes like this should not be included into patches.
> if (!input_available_p(tty, 0)) {
> up_read(&tty->termios_rwsem);
> tty_buffer_flush_work(tty->port);
>
--
i.
Hello,
kernel test robot noticed "BUG:kernel_NULL_pointer_dereference,address" on:
commit: 5c6ca526d56e87f85cf980c77f7470e76e5dd5f7 ("[PATCH] tty: fix atomicity violation in n_tty_read")
url: https://github.com/intel-lab-lkp/linux/commits/Gui-Dong-Han/tty-fix-atomicity-violation-in-n_tty_read/20240112-205942
base: https://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/all/20240112125801.2650-1-2045gemini@gmail.com/
patch subject: [PATCH] tty: fix atomicity violation in n_tty_read
in testcase: boot
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+---------------------------------------------+------------+------------+
| | 0c84bea0ca | 5c6ca526d5 |
+---------------------------------------------+------------+------------+
| boot_failures | 0 | 6 |
| BUG:kernel_NULL_pointer_dereference,address | 0 | 6 |
| Oops:#[##] | 0 | 6 |
| RIP:_raw_spin_lock_irq | 0 | 6 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 6 |
+---------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202401161610.edf0ac63-oliver.sang@intel.com
[ 53.979086][ T401] BUG: kernel NULL pointer dereference, address: 00000000000001d0
[ 53.989877][ T401] #PF: supervisor write access in kernel mode
[ 53.998103][ T401] #PF: error_code(0x0002) - not-present page
[ 54.006595][ T401] PGD 800000011f423067 P4D 800000011f423067 PUD 303527067 PMD 0
[ 54.017130][ T401] Oops: 0002 [#1] SMP PTI
[ 54.022798][ T401] CPU: 1 PID: 401 Comm: getty Not tainted 6.7.0-rc5-00199-g5c6ca526d56e #1
[ 54.032166][ T401] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 54.046197][ T401] RIP: 0010:_raw_spin_lock_irq (arch/x86/include/asm/atomic.h:115 include/linux/atomic/atomic-arch-fallback.h:2164 include/linux/atomic/atomic-instrumented.h:1296 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:187 include/linux/spinlock_api_smp.h:120 kernel/locking/spinlock.c:170)
[ 54.054135][ T401] Code: cc 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 fa 31 c0 ba 01 00 00 00 <f0> 0f b1 17 75 05 c3 cc cc cc cc 89 c6 e8 1d 01 00 00 90 c3 cc cc
All code
========
0: cc int3
1: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
8: 00
9: 90 nop
a: 90 nop
b: 90 nop
c: 90 nop
d: 90 nop
e: 90 nop
f: 90 nop
10: 90 nop
11: 90 nop
12: 90 nop
13: 90 nop
14: 90 nop
15: 90 nop
16: 90 nop
17: 90 nop
18: 90 nop
19: f3 0f 1e fa endbr64
1d: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
22: fa cli
23: 31 c0 xor %eax,%eax
25: ba 01 00 00 00 mov $0x1,%edx
2a:* f0 0f b1 17 lock cmpxchg %edx,(%rdi) <-- trapping instruction
2e: 75 05 jne 0x35
30: c3 retq
31: cc int3
32: cc int3
33: cc int3
34: cc int3
35: 89 c6 mov %eax,%esi
37: e8 1d 01 00 00 callq 0x159
3c: 90 nop
3d: c3 retq
3e: cc int3
3f: cc int3
Code starting with the faulting instruction
===========================================
0: f0 0f b1 17 lock cmpxchg %edx,(%rdi)
4: 75 05 jne 0xb
6: c3 retq
7: cc int3
8: cc int3
9: cc int3
a: cc int3
b: 89 c6 mov %eax,%esi
d: e8 1d 01 00 00 callq 0x12f
12: 90 nop
13: c3 retq
14: cc int3
15: cc int3
[ 54.079836][ T401] RSP: 0018:ffffab01406cbcd8 EFLAGS: 00010046
[ 54.088222][ T401] RAX: 0000000000000000 RBX: ffffab01406cbdc8 RCX: 0000000000000000
[ 54.098106][ T401] RDX: 0000000000000001 RSI: 0000000000000246 RDI: 00000000000001d0
[ 54.109098][ T401] RBP: ffffab01406e9000 R08: ffff9dfa447a2210 R09: ffff9dfa447a2210
[ 54.119762][ T401] R10: ffff9dfa447a2210 R11: 0000000000000000 R12: 0000000000000000
[ 54.130902][ T401] R13: 7fffffffffffffff R14: ffff9dfa447a2000 R15: 0000000000000000
[ 54.141603][ T401] FS: 0000000000000000(0000) GS:ffff9dfd2fd00000(0063) knlGS:00000000f7e1f900
[ 54.153625][ T401] CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 54.162764][ T401] CR2: 00000000000001d0 CR3: 0000000144742000 CR4: 00000000000006f0
[ 54.171900][ T401] Call Trace:
[ 54.182172][ T401] <TASK>
[ 54.186095][ T401] ? __die (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:434)
[ 54.191409][ T401] ? page_fault_oops (arch/x86/mm/fault.c:707)
[ 54.197798][ T401] ? exc_page_fault (arch/x86/include/asm/irqflags.h:37 arch/x86/include/asm/irqflags.h:72 arch/x86/mm/fault.c:1513 arch/x86/mm/fault.c:1561)
[ 54.204312][ T401] ? asm_exc_page_fault (arch/x86/include/asm/idtentry.h:570)
[ 54.211244][ T401] ? _raw_spin_lock_irq (arch/x86/include/asm/atomic.h:115 include/linux/atomic/atomic-arch-fallback.h:2164 include/linux/atomic/atomic-instrumented.h:1296 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:187 include/linux/spinlock_api_smp.h:120 kernel/locking/spinlock.c:170)
[ 54.211300][ T401] n_tty_read (drivers/tty/n_tty.c:2226)
[ 54.216906][ T401] ? __pfx_woken_wake_function (kernel/sched/wait.c:439)
[ 54.233693][ T401] tty_read (drivers/tty/tty_io.c:862 drivers/tty/tty_io.c:937)
[ 54.239403][ T401] vfs_read (include/linux/fs.h:2014 fs/read_write.c:389 fs/read_write.c:470)
[ 54.244978][ T401] ksys_read (fs/read_write.c:613)
[ 54.249805][ T401] __do_fast_syscall_32 (arch/x86/entry/common.c:165 arch/x86/entry/common.c:321)
[ 54.255574][ T401] do_fast_syscall_32 (arch/x86/entry/common.c:346)
[ 54.255607][ T401] entry_SYSENTER_compat_after_hwframe (arch/x86/entry/entry_64_compat.S:121)
[ 54.255641][ T401] RIP: 0023:0xf7fce589
[ 54.255671][ T401] Code: 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
All code
========
0: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi
...
20: 00 51 52 add %dl,0x52(%rcx)
23: 55 push %rbp
24:* 89 e5 mov %esp,%ebp <-- trapping instruction
26: 0f 34 sysenter
28: cd 80 int $0x80
2a: 5d pop %rbp
2b: 5a pop %rdx
2c: 59 pop %rcx
2d: c3 retq
2e: 90 nop
2f: 90 nop
30: 90 nop
31: 90 nop
32: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
39: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
Code starting with the faulting instruction
===========================================
0: 5d pop %rbp
1: 5a pop %rdx
2: 59 pop %rcx
3: c3 retq
4: 90 nop
5: 90 nop
6: 90 nop
7: 90 nop
8: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
f: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240116/202401161610.edf0ac63-oliver.sang@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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
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
© 2016 - 2025 Red Hat, Inc.