[syzbot] KMSAN: uninit-value in p9pdu_readf

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: c7f84f4e1147 kmsan: core: do not touch interrupts when ent..
git tree: https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=118e86a8b00000
kernel config: https://syzkaller.appspot.com/x/.config?x=978f1b2d7a5aad3e
dashboard link: https://syzkaller.appspot.com/bug?extid=06472778c97ed94af66d
compiler: clang version 14.0.0 (g...@github.com:llvm/llvm-project.git 0996585c8e3b3d409494eb5f1cad714b9e1f7fb5), GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: i386

Unfortunately, I don't have any reproducer for this issue yet.

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+064727...@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: uninit-value in p9pdu_vreadf net/9p/protocol.c:147 [inline]
BUG: KMSAN: uninit-value in p9pdu_readf+0x46cf/0x4fc0 net/9p/protocol.c:526
p9pdu_vreadf net/9p/protocol.c:147 [inline]
p9pdu_readf+0x46cf/0x4fc0 net/9p/protocol.c:526
p9pdu_vreadf net/9p/protocol.c:198 [inline]
p9pdu_readf+0x2080/0x4fc0 net/9p/protocol.c:526
p9_client_stat+0x2b3/0x710 net/9p/client.c:1724
v9fs_mount+0xc14/0x12c0 fs/9p/vfs_super.c:170
legacy_get_tree+0x163/0x2e0 fs/fs_context.c:610
vfs_get_tree+0xd8/0x5d0 fs/super.c:1498
do_new_mount+0x7bc/0x1680 fs/namespace.c:2988
path_mount+0x106f/0x2960 fs/namespace.c:3318
do_mount fs/namespace.c:3331 [inline]
__do_sys_mount fs/namespace.c:3539 [inline]
__se_sys_mount+0x8eb/0xa10 fs/namespace.c:3516
__ia32_sys_mount+0x157/0x1b0 fs/namespace.c:3516
do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
__do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
entry_SYSENTER_compat_after_hwframe+0x4d/0x5c

Local variable ----ecode@p9_check_errors created at:
p9_check_errors+0x68/0xb90 net/9p/client.c:506
p9_client_rpc+0xd90/0x1410 net/9p/client.c:801
=====================================================
Kernel panic - not syncing: panic_on_kmsan set ...
CPU: 0 PID: 31201 Comm: syz-executor.3 Tainted: G B W 5.15.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x1ff/0x28e lib/dump_stack.c:106
dump_stack+0x25/0x28 lib/dump_stack.c:113
panic+0x44f/0xdeb kernel/panic.c:232
kmsan_report+0x2ee/0x300 mm/kmsan/report.c:168
__msan_warning+0xa9/0xf0 mm/kmsan/instrumentation.c:199
p9pdu_vreadf net/9p/protocol.c:147 [inline]
p9pdu_readf+0x46cf/0x4fc0 net/9p/protocol.c:526
p9pdu_vreadf net/9p/protocol.c:198 [inline]
p9pdu_readf+0x2080/0x4fc0 net/9p/protocol.c:526
p9_client_stat+0x2b3/0x710 net/9p/client.c:1724
v9fs_mount+0xc14/0x12c0 fs/9p/vfs_super.c:170
legacy_get_tree+0x163/0x2e0 fs/fs_context.c:610
vfs_get_tree+0xd8/0x5d0 fs/super.c:1498
do_new_mount+0x7bc/0x1680 fs/namespace.c:2988
path_mount+0x106f/0x2960 fs/namespace.c:3318
do_mount fs/namespace.c:3331 [inline]
__do_sys_mount fs/namespace.c:3539 [inline]
__se_sys_mount+0x8eb/0xa10 fs/namespace.c:3516
__ia32_sys_mount+0x157/0x1b0 fs/namespace.c:3516
do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
__do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
RIP: 0023:0xf6ef3549
Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00
RSP: 002b:00000000f44ed5fc EFLAGS: 00000296 ORIG_RAX: 0000000000000015
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000020000000
RDX: 0000000020000140 RSI: 0000000000000000 RDI: 0000000020000580
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
Kernel Offset: disabled
Rebooting in 86400 seconds..
----------------
Code disassembly (best guess):
0: 03 74 c0 01 add 0x1(%rax,%rax,8),%esi
4: 10 05 03 74 b8 01 adc %al,0x1b87403(%rip) # 0x1b8740d
a: 10 06 adc %al,(%rsi)
c: 03 74 b4 01 add 0x1(%rsp,%rsi,4),%esi
10: 10 07 adc %al,(%rdi)
12: 03 74 b0 01 add 0x1(%rax,%rsi,4),%esi
16: 10 08 adc %cl,(%rax)
18: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi
1c: 00 00 add %al,(%rax)
1e: 00 00 add %al,(%rax)
20: 00 51 52 add %dl,0x52(%rcx)
23: 55 push %rbp
24: 89 e5 mov %esp,%ebp
26: 0f 34 sysenter
28: cd 80 int $0x80
* 2a: 5d pop %rbp <-- trapping instruction
2b: 5a pop %rdx
2c: 59 pop %rcx
2d: c3 retq
2e: 90 nop
2f: 90 nop
30: 90 nop
31: 90 nop
32: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
39: 00 00 00
3c: 0f .byte 0xf
3d: 1f (bad)
3e: 44 rex.R


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[Pavel Skripkin]

Looks like following code could be a culprit

p9_check_errors() {
err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
err = -ecode;
}

p9pdu_readf() can return an error and it means, that ecode wasn't
initialized.

Just for thoughts


diff --git a/net/9p/client.c b/net/9p/client.c
index 213f12ed76cd..1f697aef4ca1 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -541,6 +541,9 @@ static int p9_check_errors(struct p9_client *c,
struct p9_req_t *req)
kfree(ename);
} else {
err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
+ if (err)
+ goto out_err;
+
err = -ecode;

p9_debug(P9_DEBUG_9P, "<<< RLERROR (%d)\n", -ecode);




With regards,
Pavel Skripkin

[asma...@codewreck.org]

Question for people who know about KMSAN: which of the backtrace or the
'Local variable' message should I trust?

syzbot wrote on Sat, Oct 09, 2021 at 10:48:17PM -0700:
> =====================================================
> BUG: KMSAN: uninit-value in p9pdu_vreadf net/9p/protocol.c:147 [inline]
> BUG: KMSAN: uninit-value in p9pdu_readf+0x46cf/0x4fc0 net/9p/protocol.c:526
> p9pdu_vreadf net/9p/protocol.c:147 [inline]
> p9pdu_readf+0x46cf/0x4fc0 net/9p/protocol.c:526
> p9pdu_vreadf net/9p/protocol.c:198 [inline]
> p9pdu_readf+0x2080/0x4fc0 net/9p/protocol.c:526
> p9_client_stat+0x2b3/0x710 net/9p/client.c:1724
> v9fs_mount+0xc14/0x12c0 fs/9p/vfs_super.c:170

would be 'len' in p9pdu_vreadf, which has to be set as far as I can understand:
> uint16_t len;
>
> errcode = p9pdu_readf(pdu, proto_version,
> "w", &len);
> if (errcode)
> break;
>
> *sptr = kmalloc(len + 1, GFP_NOFS);

with relevant part of p9pdu_readf being:
> case 'w':{
> int16_t *val = va_arg(ap, int16_t *);
> __le16 le_val;
> if (pdu_read(pdu, &le_val, sizeof(le_val))) {
> errcode = -EFAULT;
> break;
> }
> *val = le16_to_cpu(le_val);
> }
> ...
> return errcode;

e.g. either len or errcode should be set...

But:

> Local variable ----ecode@p9_check_errors created at:
> p9_check_errors+0x68/0xb90 net/9p/client.c:506
> p9_client_rpc+0xd90/0x1410 net/9p/client.c:801

is something totally different, p9_client_rpc happens before the
p9pdu_readf call in p9_client_stat, and ecode is local to
p9_check_errors, I don't see how it could get that far.

Note that inspecting p9_check_errors manually, there is a case where
ecode is returned (indirectly through err = -ecode) without being
initialized, so I will send a patch for that at least, but I have no
idea if that is what has been reported and it should be trivial to
reproduce so I do not see why syzbot does not have a reproducer -- it
retries running the last program that triggered the error before sending
the report, right?

--
Dominique Martinet | Asmadeus

[Dmitry Vyukov]

On Sun, 10 Oct 2021 at 10:36, <asma...@codewreck.org> wrote:
>
> Question for people who know about KMSAN: which of the backtrace or the
> 'Local variable' message should I trust?

Hi Dominique,

Both.
The first is the use of the unit, the second is the origin of the uninit.

Does this connect both stacks? So the uinit is ecode, is it used in
p9pdu_vreadf? If yes, then that's what KMSAN reported.

> so I will send a patch for that at least, but I have no
> idea if that is what has been reported and it should be trivial to
> reproduce so I do not see why syzbot does not have a reproducer -- it
> retries running the last program that triggered the error before sending
> the report, right?

Yes.

>
> --
> Dominique Martinet | Asmadeus
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/YWKmBWfBS3oshQ/z%40codewreck.org.

[asma...@codewreck.org]

Thanks for the reply,

Hm...
Assuming that's the uninit, it'd have been propagated as the return
value as req = p9_client_rpc; passed the IS_ERR(req) check as not an
error, then passed to p9pdu_readf(&req->rc (later 'pdu')...)
That would then try to read some undefined address in pdu_read() as
memcpy(data, &pdu->sdata[pdu->offset], len)
and returning another undefined value as
sizeof(__le16) - min(pdu->size - pdu->offset, __le16)

Here magic should happen that makes this neither a success (would set
*val e.g. len in the kmalloc line that complains) or an error (would set
errcode e.g. p9pdu_vreadf() would return before reaching that line)

I guess with undefineds anything can happen and this is a valid link?


I would have assumed kmsan checks would fail sooner but I don't see what
else it could be.

> > so I will send a patch for that at least, but I have no
> > idea if that is what has been reported and it should be trivial to
> > reproduce so I do not see why syzbot does not have a reproducer -- it
> > retries running the last program that triggered the error before sending
> > the report, right?
>
> Yes.

Ok, I guess there are conditions on the undefined value to reach this
check down the road, even if the undefined return itself should be
always reproducible.

Either way Pavel Skripkin reached the same conclusion as me at roughly
the same time so I'll just go with it.

--
Dominique

[Dmitry Vyukov]

KMSAN tracks propagation of uninits and reports only "uses".
Reporting sooner tends to produce lots of false positives because code
tends to operate/propagate/copy/add uninits, but then discard.