KMSAN: uninit-value in capi_write

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: 11587f6ee534 kmsan: remove pr_err
git tree: kmsan
console output: https://syzkaller.appspot.com/x/log.txt?x=120bd84f400000
kernel config: https://syzkaller.appspot.com/x/.config?x=c8a62a4eb8ea3e9f
dashboard link: https://syzkaller.appspot.com/bug?extid=0849c524d9c634f5ae66
compiler: clang version 8.0.0 (trunk 350161)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14c53cd3400000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13e2794b400000

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

==================================================================
BUG: KMSAN: uninit-value in capi_write+0x791/0xa90
drivers/isdn/capi/capi.c:700
CPU: 0 PID: 10025 Comm: syz-executor379 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x173/0x1d0 lib/dump_stack.c:113
kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
__msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
capi_write+0x791/0xa90 drivers/isdn/capi/capi.c:700
do_loop_readv_writev fs/read_write.c:703 [inline]
do_iter_write+0x83e/0xd80 fs/read_write.c:961
vfs_writev fs/read_write.c:1004 [inline]
do_writev+0x397/0x840 fs/read_write.c:1039
__do_sys_writev fs/read_write.c:1112 [inline]
__se_sys_writev+0x9b/0xb0 fs/read_write.c:1109
__x64_sys_writev+0x4a/0x70 fs/read_write.c:1109
do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x440079
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffd84911358 EFLAGS: 00000207 ORIG_RAX: 0000000000000014
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440079
RDX: 0000000000000001 RSI: 0000000020000180 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000207 R12: 0000000000401900
R13: 0000000000401990 R14: 0000000000000000 R15: 0000000000000000

Uninit was created at:
kmsan_save_stack_with_flags mm/kmsan/kmsan.c:204 [inline]
kmsan_internal_poison_shadow+0x92/0x150 mm/kmsan/kmsan.c:158
kmsan_kmalloc+0xa6/0x130 mm/kmsan/kmsan_hooks.c:176
kmsan_slab_alloc+0xe/0x10 mm/kmsan/kmsan_hooks.c:185
slab_post_alloc_hook mm/slab.h:446 [inline]
slab_alloc_node mm/slub.c:2759 [inline]
__kmalloc_node_track_caller+0xe18/0x1030 mm/slub.c:4383
__kmalloc_reserve net/core/skbuff.c:137 [inline]
__alloc_skb+0x309/0xa20 net/core/skbuff.c:205
alloc_skb include/linux/skbuff.h:998 [inline]
capi_write+0x12f/0xa90 drivers/isdn/capi/capi.c:691
do_loop_readv_writev fs/read_write.c:703 [inline]
do_iter_write+0x83e/0xd80 fs/read_write.c:961
vfs_writev fs/read_write.c:1004 [inline]
do_writev+0x397/0x840 fs/read_write.c:1039
__do_sys_writev fs/read_write.c:1112 [inline]
__se_sys_writev+0x9b/0xb0 fs/read_write.c:1109
__x64_sys_writev+0x4a/0x70 fs/read_write.c:1109
do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7
==================================================================


---
This bug 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 bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Eric Biggers]

#syz test: https://github.com/google/kmsan.git master

From 617df11d2936c2f4334da6ab2300539e40f41ea4 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebig...@google.com>
Date: Thu, 29 Aug 2019 14:01:30 -0500
Subject: [PATCH] isdn/capi: check message length in capi_write()

syzbot reported:

BUG: KMSAN: uninit-value in capi_write+0x791/0xa90
drivers/isdn/capi/capi.c:700
CPU: 0 PID: 10025 Comm: syz-executor379 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x173/0x1d0 lib/dump_stack.c:113
kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
__msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
capi_write+0x791/0xa90 drivers/isdn/capi/capi.c:700
do_loop_readv_writev fs/read_write.c:703 [inline]
do_iter_write+0x83e/0xd80 fs/read_write.c:961
vfs_writev fs/read_write.c:1004 [inline]
do_writev+0x397/0x840 fs/read_write.c:1039
__do_sys_writev fs/read_write.c:1112 [inline]
__se_sys_writev+0x9b/0xb0 fs/read_write.c:1109
__x64_sys_writev+0x4a/0x70 fs/read_write.c:1109
do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7

[...]

The problem is that capi_write() is reading past the end of the message.
Fix it by checking the message's length in the needed places.

Reported-by: syzbot+0849c5...@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebig...@google.com>
---
drivers/isdn/capi/capi.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/isdn/capi/capi.c b/drivers/isdn/capi/capi.c
index 3c3ad42f22bf..a016d8c3c410 100644
--- a/drivers/isdn/capi/capi.c
+++ b/drivers/isdn/capi/capi.c
@@ -688,6 +688,9 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
if (!cdev->ap.applid)
return -ENODEV;

+ if (count < CAPIMSG_BASELEN)
+ return -EINVAL;
+
skb = alloc_skb(count, GFP_USER);
if (!skb)
return -ENOMEM;
@@ -698,7 +701,8 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
}
mlen = CAPIMSG_LEN(skb->data);
if (CAPIMSG_CMD(skb->data) == CAPI_DATA_B3_REQ) {
- if ((size_t)(mlen + CAPIMSG_DATALEN(skb->data)) != count) {
+ if (count < 18 ||
+ (size_t)(mlen + CAPIMSG_DATALEN(skb->data)) != count) {
kfree_skb(skb);
return -EINVAL;
}
@@ -711,6 +715,10 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
CAPIMSG_SETAPPID(skb->data, cdev->ap.applid);

if (CAPIMSG_CMD(skb->data) == CAPI_DISCONNECT_B3_RESP) {
+ if (count < 12) {
+ kfree_skb(skb);
+ return -EINVAL;
+ }
mutex_lock(&cdev->lock);
capincci_free(cdev, CAPIMSG_NCCI(skb->data));
mutex_unlock(&cdev->lock);
--
2.23.0

[Eric Biggers]

On Thu, Aug 29, 2019 at 02:34:29PM -0500, Eric Biggers wrote:
> #syz test: https://github.com/google/kmsan.git master
>
> From 617df11d2936c2f4334da6ab2300539e40f41ea4 Mon Sep 17 00:00:00 2001
> From: Eric Biggers <ebig...@google.com>
> Date: Thu, 29 Aug 2019 14:01:30 -0500
> Subject: [PATCH] isdn/capi: check message length in capi_write()
>
> syzbot reported:
>

[...]

I haven't gotten any response to this from syzbot. Maybe a bug?

- Eric

[Dmitry Vyukov]

syzbot is bisecting this
https://syzkaller.appspot.com/bug?id=ea8102c6219a001c57f08263afc16d2eae6376bc
for the past 2 days
2 patch testing requests are queued and will be handled when bisectoin finishes

[Eric Biggers]

Why is that bisection taking so long? Also, shouldn't human requests be handled
in parallel with background work, so people don't have to wait days?

BTW, I also tried to test this myself, but KMSAN is boot-broken for me:
https://github.com/google/kmsan/issues/57

- Eric

[Dmitry Vyukov]

On Fri, Aug 30, 2019 at 6:59 PM Eric Biggers <ebig...@kernel.org> wrote:
>
> On Fri, Aug 30, 2019 at 06:45:32PM -0700, 'Dmitry Vyukov' via syzkaller-bugs wrote:
> > On Fri, Aug 30, 2019 at 5:04 PM Eric Biggers <ebig...@kernel.org> wrote:
> > >
> > > On Thu, Aug 29, 2019 at 02:34:29PM -0500, Eric Biggers wrote:
> > > > #syz test: https://github.com/google/kmsan.git master
> > > >
> > > > From 617df11d2936c2f4334da6ab2300539e40f41ea4 Mon Sep 17 00:00:00 2001
> > > > From: Eric Biggers <ebig...@google.com>
> > > > Date: Thu, 29 Aug 2019 14:01:30 -0500
> > > > Subject: [PATCH] isdn/capi: check message length in capi_write()
> > > >
> > > > syzbot reported:
> > > >
> > > [...]
> > >
> > > I haven't gotten any response to this from syzbot. Maybe a bug?
> >
> > syzbot is bisecting this
> > https://syzkaller.appspot.com/bug?id=ea8102c6219a001c57f08263afc16d2eae6376bc
> > for the past 2 days
> > 2 patch testing requests are queued and will be handled when bisectoin finishes
> >
>
> Why is that bisection taking so long?

The ones that I looked at before took long because of lots of
build/boot broken commits. git bisect skip seems to be linear in
number of commits.

> Also, shouldn't human requests be handled
> in parallel with background work, so people don't have to wait days?

Probably better run in parallel.

[Dmitry Vyukov]

On Fri, Aug 30, 2019 at 7:20 PM Dmitry Vyukov <dvy...@google.com> wrote:
> > > > On Thu, Aug 29, 2019 at 02:34:29PM -0500, Eric Biggers wrote:
> > > > > #syz test: https://github.com/google/kmsan.git master
> > > > >
> > > > > From 617df11d2936c2f4334da6ab2300539e40f41ea4 Mon Sep 17 00:00:00 2001
> > > > > From: Eric Biggers <ebig...@google.com>
> > > > > Date: Thu, 29 Aug 2019 14:01:30 -0500
> > > > > Subject: [PATCH] isdn/capi: check message length in capi_write()
> > > > >
> > > > > syzbot reported:
> > > > >
> > > > [...]
> > > >
> > > > I haven't gotten any response to this from syzbot. Maybe a bug?
> > >
> > > syzbot is bisecting this
> > > https://syzkaller.appspot.com/bug?id=ea8102c6219a001c57f08263afc16d2eae6376bc
> > > for the past 2 days
> > > 2 patch testing requests are queued and will be handled when bisectoin finishes
> > >
> >
> > Why is that bisection taking so long?
>
> The ones that I looked at before took long because of lots of
> build/boot broken commits. git bisect skip seems to be linear in
> number of commits.
>
> > Also, shouldn't human requests be handled
> > in parallel with background work, so people don't have to wait days?
>
> Probably better run in parallel.

What happened there is an interesting case.

A recent change (probably "btf: expose BTF info through sysfs" or some
related) made kernel build require pahole binary. syzbot machines did
not have it, so all kernel builds started failing with:

LD vmlinux
scripts/link-vmlinux.sh: line 99: pahole: command not found
scripts/link-vmlinux.sh: line 100: [: : integer expression expected
BTF vmlinux
scripts/link-vmlinux.sh: line 106: pahole: command not found
Makefile:1030: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 127

and git bisect skip behavior is linear, so it was trying to build
kernel on all recent commits for 3 days.
I now installed a required package, so it should unstuck soon.

However, it also brings an interesting question: such changes break
all kernel testing out there, and we don't have any way to coordinate
roll out. As a result we are doomed to break everything on each such
change and then waste lots of human's time...
The same happens with kernel crash format changes too...

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
syzbot+0849c5...@syzkaller.appspotmail.com

Tested on:

commit: e88bfb81 kmsan_entry.c: drop an unnecessary #include
git tree: https://github.com/google/kmsan.git master
kernel config: https://syzkaller.appspot.com/x/.config?x=c455ebddbe2c3688
dashboard link: https://syzkaller.appspot.com/bug?extid=0849c524d9c634f5ae66
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
patch: https://syzkaller.appspot.com/x/patch.diff?x=127f7fd2600000

Note: testing is done by a robot and is best-effort only.

[Eric Biggers]

From: Eric Biggers <ebig...@google.com>

syzbot reported:

BUG: KMSAN: uninit-value in capi_write+0x791/0xa90 drivers/isdn/capi/capi.c:700
CPU: 0 PID: 10025 Comm: syz-executor379 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x173/0x1d0 lib/dump_stack.c:113
kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
__msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
capi_write+0x791/0xa90 drivers/isdn/capi/capi.c:700
do_loop_readv_writev fs/read_write.c:703 [inline]
do_iter_write+0x83e/0xd80 fs/read_write.c:961
vfs_writev fs/read_write.c:1004 [inline]
do_writev+0x397/0x840 fs/read_write.c:1039
__do_sys_writev fs/read_write.c:1112 [inline]
__se_sys_writev+0x9b/0xb0 fs/read_write.c:1109
__x64_sys_writev+0x4a/0x70 fs/read_write.c:1109
do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7

[...]

The problem is that capi_write() is reading past the end of the message.
Fix it by checking the message's length in the needed places.

Reported-and-tested-by: syzbot+0849c5...@syzkaller.appspotmail.com

[David Miller]

From: Eric Biggers <ebig...@kernel.org>
Date: Sun, 1 Sep 2019 09:32:39 -0500

This is fine.

> @@ -698,7 +701,8 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
> }
> mlen = CAPIMSG_LEN(skb->data);
> if (CAPIMSG_CMD(skb->data) == CAPI_DATA_B3_REQ) {
> - if ((size_t)(mlen + CAPIMSG_DATALEN(skb->data)) != count) {
> + if (count < 18 ||

...
> + if (count < 12) {

These magic constants, on the other hand, are not.

[Eric Biggers]

Changed v1 => v2:

- Use CAPI_DATA_B3_REQ_LEN, and define and use a constant for
CAPI_DISCONNECT_B3_RESP_LEN.

drivers/isdn/capi/capi.c | 10 +++++++++-
include/uapi/linux/isdn/capicmd.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/isdn/capi/capi.c b/drivers/isdn/capi/capi.c
index 3c3ad42f22bf..c92b405b7646 100644

--- a/drivers/isdn/capi/capi.c
+++ b/drivers/isdn/capi/capi.c
@@ -688,6 +688,9 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
if (!cdev->ap.applid)
return -ENODEV;

+ if (count < CAPIMSG_BASELEN)
+ return -EINVAL;
+
skb = alloc_skb(count, GFP_USER);
if (!skb)
return -ENOMEM;

@@ -698,7 +701,8 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
}
mlen = CAPIMSG_LEN(skb->data);
if (CAPIMSG_CMD(skb->data) == CAPI_DATA_B3_REQ) {
- if ((size_t)(mlen + CAPIMSG_DATALEN(skb->data)) != count) {

+ if (count < CAPI_DATA_B3_REQ_LEN ||

+ (size_t)(mlen + CAPIMSG_DATALEN(skb->data)) != count) {
kfree_skb(skb);
return -EINVAL;
}
@@ -711,6 +715,10 @@ capi_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos
CAPIMSG_SETAPPID(skb->data, cdev->ap.applid);

if (CAPIMSG_CMD(skb->data) == CAPI_DISCONNECT_B3_RESP) {

+ if (count < CAPI_DISCONNECT_B3_RESP_LEN) {

+ kfree_skb(skb);
+ return -EINVAL;
+ }
mutex_lock(&cdev->lock);
capincci_free(cdev, CAPIMSG_NCCI(skb->data));
mutex_unlock(&cdev->lock);

diff --git a/include/uapi/linux/isdn/capicmd.h b/include/uapi/linux/isdn/capicmd.h
index 4941628a4fb9..5ec88e7548a9 100644
--- a/include/uapi/linux/isdn/capicmd.h
+++ b/include/uapi/linux/isdn/capicmd.h
@@ -16,6 +16,7 @@
#define CAPI_MSG_BASELEN 8
#define CAPI_DATA_B3_REQ_LEN (CAPI_MSG_BASELEN+4+4+2+2+2)
#define CAPI_DATA_B3_RESP_LEN (CAPI_MSG_BASELEN+4+2)
+#define CAPI_DISCONNECT_B3_RESP_LEN (CAPI_MSG_BASELEN+4)

/*----- CAPI commands -----*/
#define CAPI_ALERT 0x01
--
2.23.0

[David Miller]

From: Eric Biggers <ebig...@kernel.org>
Date: Thu, 5 Sep 2019 19:36:37 -0700

Applied and queued up for -stable.

[Andrii Nakryiko]

On 8/31/19 10:11 PM, Dmitry Vyukov wrote:
> On Fri, Aug 30, 2019 at 7:20 PM Dmitry Vyukov <dvy...@google.com> wrote:
>>>>> On Thu, Aug 29, 2019 at 02:34:29PM -0500, Eric Biggers wrote:
>>>>>> #syz test: https://github.com/google/kmsan.git master
>>>>>>
>>>>>> From 617df11d2936c2f4334da6ab2300539e40f41ea4 Mon Sep 17 00:00:00 2001
>>>>>> From: Eric Biggers <ebig...@google.com>
>>>>>> Date: Thu, 29 Aug 2019 14:01:30 -0500
>>>>>> Subject: [PATCH] isdn/capi: check message length in capi_write()
>>>>>>
>>>>>> syzbot reported:
>>>>>>
>>>>> [...]
>>>>>
>>>>> I haven't gotten any response to this from syzbot. Maybe a bug?
>>>>
>>>> syzbot is bisecting this

>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fid-3Dea8102c6219a001c57f08263afc16d2eae6376bc&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=vxqvl81C2rT6GOGdPyz8iQ&m=yoTNc2J7UDs50Y_ClEk2c87w__62WC0yQz-k0Tzbpno&s=CkMfLMy-tGHXuIfm9RAqP3_nRI_RYysElDhyIqnFb1U&e=

>>>> for the past 2 days
>>>> 2 patch testing requests are queued and will be handled when bisectoin finishes
>>>>
>>>
>>> Why is that bisection taking so long?
>>
>> The ones that I looked at before took long because of lots of
>> build/boot broken commits. git bisect skip seems to be linear in
>> number of commits.
>>
>>> Also, shouldn't human requests be handled
>>> in parallel with background work, so people don't have to wait days?
>>
>> Probably better run in parallel.
>
> What happened there is an interesting case.
>
> A recent change (probably "btf: expose BTF info through sysfs" or some
> related) made kernel build require pahole binary. syzbot machines did
> not have it, so all kernel builds started failing with:

Sorry for late reply, just digged deep enough through my email backlog...

The change you are referring to didn't change anything about pahole
requirement. The original commit that added dependency on pahole was
e83b9f55448a ("kbuild: add ability to generate BTF type info for
vmlinux") from April 2nd. The intent was always to gracefully handle
missing or old pahole and not cause build failure, but unfortunately
original commit handled old pahole case, but not missing case. This was
fixed on May 5th with 581b31c36cfc ("kbuild: tolerate missing pahole
when generating BTF").

So I'm surprised this failure happened recently. Was this just not
noticed before?

>
> LD vmlinux
> scripts/link-vmlinux.sh: line 99: pahole: command not found
> scripts/link-vmlinux.sh: line 100: [: : integer expression expected
> BTF vmlinux
> scripts/link-vmlinux.sh: line 106: pahole: command not found
> Makefile:1030: recipe for target 'vmlinux' failed
> make: *** [vmlinux] Error 127
>
> and git bisect skip behavior is linear, so it was trying to build
> kernel on all recent commits for 3 days.
> I now installed a required package, so it should unstuck soon.

Please let me know if this still happens with 581b31c36cfc ("kbuild:
tolerate missing pahole when generating BTF") commit in sources, missing
pahole shouldn't be breaking the build. I verified locally and it works
as expected:

...
BTF .tmp_vmlinux.btf: pahole (pahole) is not available
KSYM .tmp_kallsyms1.o
KSYM .tmp_kallsyms2.o
LD vmlinux
SORTEX vmlinux
...

>
> However, it also brings an interesting question: such changes break
> all kernel testing out there, and we don't have any way to coordinate
> roll out. As a result we are doomed to break everything on each such
> change and then waste lots of human's time...

In this particular case you would see this problem only with
CONFIG_DEBUG_INFO_BTF=y config and only until 581b31c36cfc ("kbuild:
tolerate missing pahole when generating BTF"). I apologize that my bug
caused troubles for you! But it seems like the overall idea of
gracefully skipping BTF generation step if pahole is missing/old is sane
and works good (after a bug fix, sigh...).

[Dmitry Vyukov]

Ah, this makes sense. Thanks for the info.
I guess what happened then is that during bisection syzbot hit that
range where the build was broken, and since git bisect is linear when
commits are skipped, it stuck in that range for days trying to get any
successful build.
Yes, everything seems to be working fine on head.