kernel BUG at lib/string.c:LINE! (6)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: d64c6f96 Merge tag 'net-5.11-rc1' of git://git.kernel.org/..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10bc5613500000
kernel config: https://syzkaller.appspot.com/x/.config?x=aca0dc5c721fe9e5
dashboard link: https://syzkaller.appspot.com/bug?extid=e86f7c428c8c50db65b4
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=169378a7500000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=144692cb500000

The issue was bisected to:

commit 2f78788b55baa3410b1ec91a576286abe1ad4d6a
Author: Jakub Jelinek <ja...@redhat.com>
Date: Wed Dec 16 04:43:37 2020 +0000

ilog2: improve ilog2 for constant arguments

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1584f137500000
final oops: https://syzkaller.appspot.com/x/report.txt?x=1784f137500000
console output: https://syzkaller.appspot.com/x/log.txt?x=1384f137500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+e86f7c...@syzkaller.appspotmail.com
Fixes: 2f78788b55ba ("ilog2: improve ilog2 for constant arguments")

detected buffer overflow in strlen
------------[ cut here ]------------
kernel BUG at lib/string.c:1149!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 8713 Comm: syz-executor731 Not tainted 5.10.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:fortify_panic+0xf/0x11 lib/string.c:1149
Code: b5 78 a3 04 48 c7 c7 c0 8f c2 89 58 5b 5d 41 5c 41 5d 41 5e 41 5f e9 30 ba ee ff 48 89 fe 48 c7 c7 80 90 c2 89 e8 21 ba ee ff <0f> 0b e8 90 f9 97 f8 0f b6 f3 48 c7 c7 20 f4 10 8c e8 41 e8 fc fa
RSP: 0018:ffffc900020af500 EFLAGS: 00010282
RAX: 0000000000000022 RBX: ffff888011c26768 RCX: 0000000000000000
RDX: ffff88801bad0000 RSI: ffffffff815a6925 RDI: fffff52000415e92
RBP: ffff88801be7c220 R08: 0000000000000022 R09: 0000000000000000
R10: ffffffff815a4d7b R11: 0000000000000000 R12: ffff88801180ec00
R13: ffff888011c26700 R14: 1ffff92000415ea2 R15: 0000000000000010
FS: 0000000000812880(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000006dcf60 CR3: 00000000141ee000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
strlen include/linux/string.h:325 [inline]
strlcpy include/linux/string.h:348 [inline]
xt_rateest_tg_checkentry+0x2a5/0x6b0 net/netfilter/xt_RATEEST.c:143
xt_check_target+0x26c/0x9e0 net/netfilter/x_tables.c:1019
check_target net/ipv6/netfilter/ip6_tables.c:529 [inline]
find_check_entry.constprop.0+0x7f1/0x9e0 net/ipv6/netfilter/ip6_tables.c:572
translate_table+0xc8b/0x1750 net/ipv6/netfilter/ip6_tables.c:734
do_replace net/ipv6/netfilter/ip6_tables.c:1152 [inline]
do_ip6t_set_ctl+0x553/0xb70 net/ipv6/netfilter/ip6_tables.c:1636
nf_setsockopt+0x83/0xe0 net/netfilter/nf_sockopt.c:101
ipv6_setsockopt+0x122/0x180 net/ipv6/ipv6_sockglue.c:1008
tcp_setsockopt+0x136/0x2440 net/ipv4/tcp.c:3597
__sys_setsockopt+0x2db/0x610 net/socket.c:2115
__do_sys_setsockopt net/socket.c:2126 [inline]
__se_sys_setsockopt net/socket.c:2123 [inline]
__x64_sys_setsockopt+0xba/0x150 net/socket.c:2123
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4493d9
Code: e8 0c ca 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 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 9b cb fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fff679a3898 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
RAX: ffffffffffffffda RBX: 00000000200002c0 RCX: 00000000004493d9
RDX: 0000000000000040 RSI: 0000000000000029 RDI: 0000000000000006
RBP: 00007fff679a38b0 R08: 0000000000000470 R09: 00000000000000c2
R10: 0000000020000080 R11: 0000000000000246 R12: 00000000000112d5
R13: 00000000006d7dc8 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace e17a915ca7e8b666 ]---
RIP: 0010:fortify_panic+0xf/0x11 lib/string.c:1149
Code: b5 78 a3 04 48 c7 c7 c0 8f c2 89 58 5b 5d 41 5c 41 5d 41 5e 41 5f e9 30 ba ee ff 48 89 fe 48 c7 c7 80 90 c2 89 e8 21 ba ee ff <0f> 0b e8 90 f9 97 f8 0f b6 f3 48 c7 c7 20 f4 10 8c e8 41 e8 fc fa
RSP: 0018:ffffc900020af500 EFLAGS: 00010282
RAX: 0000000000000022 RBX: ffff888011c26768 RCX: 0000000000000000
RDX: ffff88801bad0000 RSI: ffffffff815a6925 RDI: fffff52000415e92
RBP: ffff88801be7c220 R08: 0000000000000022 R09: 0000000000000000
R10: ffffffff815a4d7b R11: 0000000000000000 R12: ffff88801180ec00
R13: ffff888011c26700 R14: 1ffff92000415ea2 R15: 0000000000000010
FS: 0000000000812880(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000006dcf60 CR3: 00000000141ee000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
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.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Linus Torvalds]

On Tue, Dec 22, 2020 at 6:44 AM syzbot
<syzbot+e86f7c...@syzkaller.appspotmail.com> wrote:
>
> The issue was bisected to:
>

> commit 2f78788b55ba ("ilog2: improve ilog2 for constant arguments")

That looks unlikely, although possibly some constant folding
improvement might make the fortify code notice something with it.

> detected buffer overflow in strlen
> ------------[ cut here ]------------
> kernel BUG at lib/string.c:1149!

> Call Trace:
> strlen include/linux/string.h:325 [inline]
> strlcpy include/linux/string.h:348 [inline]
> xt_rateest_tg_checkentry+0x2a5/0x6b0 net/netfilter/xt_RATEEST.c:143

Honestly, this just looks like the traditional bug in "strlcpy()".

That BSD function is complete garbage, exactly because it doesn't
limit the source length. People tend to _think_ it does ("what's that
size_t argument for?") but strlcpy() only limits the *destination*
size, and the source is always read fully.

So it's a completely useless function if you can't implicitly trust
the source string - but that is almost always why people think they
should use it!

Nobody should use it. I really would like to remove it, and let
everybody know how incredibly broken sh*t that function is.

Can we please have everybody stop using strlcpy(). But in this
particular case, it's that xt_rateest_tg_checkentry() in
net/netfilter/xt_RATEEST.c

That said, this may be a real FORTIFY report if that info->name is
*supposed* to be trustworthy? The xt_RATETEST code does use
"info->name" a few lines earlier when it does

est = __xt_rateest_lookup(xn, info->name);

or maybe the bisection is right, and this points to some problem with
__builtin_clzll?

Linus

[Florian Westphal]

Linus Torvalds <torv...@linux-foundation.org> wrote:
> On Tue, Dec 22, 2020 at 6:44 AM syzbot
> <syzbot+e86f7c...@syzkaller.appspotmail.com> wrote:
> >
> > The issue was bisected to:
> >
> > commit 2f78788b55ba ("ilog2: improve ilog2 for constant arguments")
>
> That looks unlikely, although possibly some constant folding
> improvement might make the fortify code notice something with it.
>
> > detected buffer overflow in strlen
> > ------------[ cut here ]------------
> > kernel BUG at lib/string.c:1149!
> > Call Trace:
> > strlen include/linux/string.h:325 [inline]
> > strlcpy include/linux/string.h:348 [inline]
> > xt_rateest_tg_checkentry+0x2a5/0x6b0 net/netfilter/xt_RATEEST.c:143
>
> Honestly, this just looks like the traditional bug in "strlcpy()".

Yes, thats exactly what this is, no idea why the bisection points
at ilog2 changes.

> That BSD function is complete garbage, exactly because it doesn't
> limit the source length. People tend to _think_ it does ("what's that
> size_t argument for?") but strlcpy() only limits the *destination*
> size, and the source is always read fully.

Right, I'll send a patch shortly.

[Florian Westphal]

syzbot reports:

detected buffer overflow in strlen

[..]

Call Trace:
strlen include/linux/string.h:325 [inline]
strlcpy include/linux/string.h:348 [inline]
xt_rateest_tg_checkentry+0x2a5/0x6b0 net/netfilter/xt_RATEEST.c:143

strlcpy assumes src is a c-string. Check info->name before its used.

Reported-by: syzbot+e86f7c...@syzkaller.appspotmail.com
Fixes: 5859034d7eb8793 ("[NETFILTER]: x_tables: add RATEEST target")
Signed-off-by: Florian Westphal <f...@strlen.de>
---
RATEEST test in iptables.git still passes, syzbot repro setsockopt
fails with -ENAMETOOLONG.

diff --git a/net/netfilter/xt_RATEEST.c b/net/netfilter/xt_RATEEST.c
index 37253d399c6b..0d5c422f8745 100644
--- a/net/netfilter/xt_RATEEST.c
+++ b/net/netfilter/xt_RATEEST.c
@@ -115,6 +115,9 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par)
} cfg;
int ret;

+ if (strnlen(info->name, sizeof(est->name)) >= sizeof(est->name))
+ return -ENAMETOOLONG;
+
net_get_random_once(&jhash_rnd, sizeof(jhash_rnd));

mutex_lock(&xn->hash_lock);
--
2.26.2

[Linus Torvalds]

On Tue, Dec 22, 2020 at 2:24 PM Florian Westphal <f...@strlen.de> wrote:
>
> strlcpy assumes src is a c-string. Check info->name before its used.

If strlcpy is the only problem, then the fix is to use strscpy(),
which doesn't have the design mistake that strlcpy has.

Of course, if the size limit of the source and the destination differ
(ie if you really want to limit the source to one thing, and the
destination to another - there are in theory valid cases where that
happens), then there are no useful helper functions for that.

Linus

[Florian Westphal]

Linus Torvalds <torv...@linux-foundation.org> wrote:
> On Tue, Dec 22, 2020 at 2:24 PM Florian Westphal <f...@strlen.de> wrote:
> >
> > strlcpy assumes src is a c-string. Check info->name before its used.
>
> If strlcpy is the only problem, then the fix is to use strscpy(),
> which doesn't have the design mistake that strlcpy has.

It would silence the reproducer, but the checkentry function calls
__xt_rateest_lookup which may 'strcmp(..., maybe_not_zero_terminated)'.

[Dmitry Vyukov]

On Tue, Dec 22, 2020 at 11:07 PM Florian Westphal <f...@strlen.de> wrote:
>
> Linus Torvalds <torv...@linux-foundation.org> wrote:
> > On Tue, Dec 22, 2020 at 6:44 AM syzbot
> > <syzbot+e86f7c...@syzkaller.appspotmail.com> wrote:
> > >
> > > The issue was bisected to:
> > >
> > > commit 2f78788b55ba ("ilog2: improve ilog2 for constant arguments")
> >
> > That looks unlikely, although possibly some constant folding
> > improvement might make the fortify code notice something with it.
> >
> > > detected buffer overflow in strlen
> > > ------------[ cut here ]------------
> > > kernel BUG at lib/string.c:1149!
> > > Call Trace:
> > > strlen include/linux/string.h:325 [inline]
> > > strlcpy include/linux/string.h:348 [inline]
> > > xt_rateest_tg_checkentry+0x2a5/0x6b0 net/netfilter/xt_RATEEST.c:143
> >
> > Honestly, this just looks like the traditional bug in "strlcpy()".
>
> Yes, thats exactly what this is, no idea why the bisection points
> at ilog2 changes.

The end result is usually clear from the bisection log:

> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1584f137500000

In this case it looks like the most common cause of diverted bisection
-- interference from other kernel bugs, this __queue_work issue that
happened on ilog2 commit:

[03f4935135b9efeb780b970ba023c201f81cf4e6] checkpatch: fix unescaped left brace
testing commit 03f4935135b9efeb780b970ba023c201f81cf4e6 with gcc (GCC) 8.1.0
all runs: crashed: kernel BUG at lib/string.c:LINE!
# git bisect bad 03f4935135b9efeb780b970ba023c201f81cf4e6

Bisecting: 21 revisions left to test after this (roughly 5 steps)
[2f78788b55baa3410b1ec91a576286abe1ad4d6a] ilog2: improve ilog2 for
constant arguments
testing commit 2f78788b55baa3410b1ec91a576286abe1ad4d6a with gcc (GCC) 8.1.0
run #0: crashed: WARNING in __queue_work
# git bisect bad 2f78788b55baa3410b1ec91a576286abe1ad4d6a

[Pablo Neira Ayuso]

On Tue, Dec 22, 2020 at 11:23:56PM +0100, Florian Westphal wrote:
> syzbot reports:
> detected buffer overflow in strlen
> [..]
> Call Trace:
> strlen include/linux/string.h:325 [inline]
> strlcpy include/linux/string.h:348 [inline]
> xt_rateest_tg_checkentry+0x2a5/0x6b0 net/netfilter/xt_RATEEST.c:143
>
> strlcpy assumes src is a c-string. Check info->name before its used.

Applied, thanks Florian.