KASAN: use-after-free Read in rds_tcp_tune

[syzbot]

Hello,

syzkaller hit the following crash on
8418f88764046d0e8ca6a3c04a69a0e57189aa1e
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
Unfortunately, I don't have any reproducer for this bug yet.


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+bbd8e9...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

sock: sock_set_timeout: `syz-executor3' (pid 12396) tries to set negative
timeout
==================================================================
BUG: KASAN: use-after-free in rds_tcp_tune+0x491/0x520 net/rds/tcp.c:397
Read of size 4 at addr ffff8801cd5f6c58 by task kworker/u4:4/4954

CPU: 1 PID: 4954 Comm: kworker/u4:4 Not tainted 4.15.0-rc7-next-20180111+
#94
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: krdsd rds_connect_worker
Call Trace:
__dump_stack lib/dump_stack.c:17 [inline]
dump_stack+0x194/0x257 lib/dump_stack.c:53
print_address_description+0x73/0x250 mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report+0x23b/0x360 mm/kasan/report.c:412
__asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:432
rds_tcp_tune+0x491/0x520 net/rds/tcp.c:397
rds_tcp_conn_path_connect+0x397/0x940 net/rds/tcp_connect.c:113
rds_connect_worker+0x156/0x1f0 net/rds/threads.c:175
process_one_work+0xbbf/0x1af0 kernel/workqueue.c:2112
worker_thread+0x223/0x1990 kernel/workqueue.c:2246
kthread+0x33c/0x400 kernel/kthread.c:238
ret_from_fork+0x4b/0x60 arch/x86/entry/entry_64.S:547

Allocated by task 3743:
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459 [inline]
kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:552
__do_kmalloc mm/slab.c:3705 [inline]
__kmalloc+0x162/0x760 mm/slab.c:3714
kmalloc include/linux/slab.h:520 [inline]
kzalloc include/linux/slab.h:704 [inline]
ops_init+0x172/0x570 net/core/net_namespace.c:108
setup_net+0x313/0x710 net/core/net_namespace.c:295
copy_net_ns+0x27c/0x580 net/core/net_namespace.c:419
create_new_namespaces+0x425/0x880 kernel/nsproxy.c:107
unshare_nsproxy_namespaces+0xae/0x1e0 kernel/nsproxy.c:206
SYSC_unshare kernel/fork.c:2415 [inline]
SyS_unshare+0x653/0xfa0 kernel/fork.c:2365
entry_SYSCALL_64_fastpath+0x29/0xa0

Freed by task 246:
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459 [inline]
__kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:520
kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:527
__cache_free mm/slab.c:3485 [inline]
kfree+0xd9/0x260 mm/slab.c:3800
ops_free net/core/net_namespace.c:132 [inline]
ops_free_list.part.8+0x27e/0x3e0 net/core/net_namespace.c:154
ops_free_list net/core/net_namespace.c:152 [inline]
cleanup_net+0x665/0xb50 net/core/net_namespace.c:488
process_one_work+0xbbf/0x1af0 kernel/workqueue.c:2112
worker_thread+0x223/0x1990 kernel/workqueue.c:2246
kthread+0x33c/0x400 kernel/kthread.c:238
ret_from_fork+0x4b/0x60 arch/x86/entry/entry_64.S:547

The buggy address belongs to the object at ffff8801cd5f6c00
which belongs to the cache kmalloc-96 of size 96
The buggy address is located 88 bytes inside of
96-byte region [ffff8801cd5f6c00, ffff8801cd5f6c60)
The buggy address belongs to the page:
page:ffffea0007357d80 count:1 mapcount:0 mapping:ffff8801cd5f6000 index:0x0
flags: 0x2fffc0000000100(slab)
raw: 02fffc0000000100 ffff8801cd5f6000 0000000000000000 0000000100000020
raw: ffffea000735ca20 ffffea000735e7e0 ffff8801dac004c0 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff8801cd5f6b00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
ffff8801cd5f6b80: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> ffff8801cd5f6c00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
^
ffff8801cd5f6c80: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
ffff8801cd5f6d00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
==================================================================


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.

syzbot will keep track of this bug report.
If you forgot to add the Reported-by tag, once the fix for this bug is
merged
into any tree, please reply to this email with:
#syz fix: exact-commit-title
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line in the email body.

[Sowmini Varadhan]

On (01/11/18 21:29), syzbot wrote:
> ==================================================================
> BUG: KASAN: use-after-free in rds_tcp_tune+0x491/0x520 net/rds/tcp.c:397
> Read of size 4 at addr ffff8801cd5f6c58 by task kworker/u4:4/4954

Just had an offline discussion with santosh around this, here's a summary
of that discussion for the archives:

Looks like an rds_connect_worker workq got scheduled after the
netns was deleted. This could happen if an an rds_connection got
added between lines 528 and 529 of

506 static void rds_tcp_kill_sock(struct net *net)
:
/* code to pull out all the rds_connections that should be destroyed */
:
528 spin_unlock_irq(&rds_tcp_conn_lock);
529 list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
530 rds_conn_destroy(tc->t_cpath->cp_conn);

Such an rds_connection would miss out the rds_conn_destroy()
loop (that cancels all pending work) and (if it was scheduled
after netns deletion) could trigger the use-after-free.

Evaluating various fixes for this (including using _bh instead of _irq
as suggested by santosh), I'll get back with a patch soon.

--Sowmini

[Dmitry Vyukov]

Hi Sowmini,

Was this ever fixed? What's the fix? This still hangs as open. Please
provide "syz fix" tag.

[Sowmini Varadhan]

On (02/14/18 16:11), Dmitry Vyukov wrote:
>
> Hi Sowmini,
>
> Was this ever fixed? What's the fix? This still hangs as open. Please
> provide "syz fix" tag.

Are you still seeing this problem?

I had expected that the changes around rds_destroy_pending - see commit
ebeeb1ad9b8a - would have taken care of this (note that ebeeb1ad9b8a
refactors/updates 3db6e0d172c9) but those fixes were done by inspection
only. In other words, I was never able to reproduce this, so we may
still have missed some race condition.

--Sowmini

[Dmitry Vyukov]

syzbot is probably not seeing this problem. However if you don't add
the Reported-by tag to commit, nor provide syz fix tag, it will
consider it as "open". One consequence of this is that it is still on
our radars. Another consequence is that syzbot will never report bugs
in rds_tcp_tune ever again as it thinks that it's the same known bug,
so no point in bothering anybody.

[Sowmini Varadhan]

On (02/14/18 16:28), Dmitry Vyukov wrote:
> syzbot is probably not seeing this problem. However if you don't add
> the Reported-by tag to commit, nor provide syz fix tag, it will
> consider it as "open". One consequence of this is that it is still on
> our radars. Another consequence is that syzbot will never report bugs
> in rds_tcp_tune ever again as it thinks that it's the same known bug,
> so no point in bothering anybody.

understood, I think I saw this in the original syzbot mail as well,
but I was hesitant to actually add the tag because the fix was
based on code-inspection only, and I would have felt more comfortable
about asserting the Reported-by if I'd done a clear-cut before/after
verification.

btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com
addresses as "Unrecognized email address", we should fix that
error from checkpatch at some point.

--Sowmini

[Dmitry Vyukov]

Interesting. Looking at checkpatch.pl I think it wants all addresses
to be in <>, i.e.
Reported-by: <syzbot+bbd8e9...@syzkaller.appspotmail.com>
There probably was some reason to enforce this, so I think I will
change the syzbot email template to include <>.
Thanks!

[Joe Perches]

Not really.

It's the somewhat unusual + in the address
that perl needs quoted before a substitution.

I believe this fixes it in checkpatch.
---
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3d4040322ae1..2b8397da39d3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1075,7 +1075,7 @@ sub parse_email {
} elsif ($formatted_email =~ /(\S+\@\S+)(.*)$/) {
$address = $1;
$comment = $2 if defined $2;
- $formatted_email =~ s/$address.*$//;
+ $formatted_email =~ s/\Q$address\E.*$//;
$name = $formatted_email;
$name = trim($name);
$name =~ s/^\"|\"$//g;

[Dmitry Vyukov]

I can confirm that running
$ git show HEAD | scripts/checkpatch.pl -
on a commit that contains a syzbot Reported-by tag does not produce
the warning anymore.

Joe, do you mind mailing this as patch?

[Joe Perches]

On Wed, 2018-02-14 at 18:16 +0100, Dmitry Vyukov wrote:
> On Wed, Feb 14, 2018 at 6:02 PM, Joe Perches <j...@perches.com> wrote:
> > On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote:
> > > On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan <sowmini....@oracle.com> wrote:
> > > > btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com
> > > > addresses as "Unrecognized email address", we should fix that
> > > > error from checkpatch at some point.
> > >
> > > Interesting. Looking at checkpatch.pl I think it wants all addresses
> > > to be in <>, i.e.
> > > Reported-by: <syzbot+bbd8e9...@syzkaller.appspotmail.com>
> > > There probably was some reason to enforce this, so I think I will
> > > change the syzbot email template to include <>.
> > > Thanks!
> >
> > Not really.
> >
> > It's the somewhat unusual + in the address
> > that perl needs quoted before a substitution.
> >
> > I believe this fixes it in checkpatch.
> > ---
> > scripts/checkpatch.pl | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

[]

> > @@ -1075,7 +1075,7 @@ sub parse_email {
> > } elsif ($formatted_email =~ /(\S+\@\S+)(.*)$/) {
> > $address = $1;
> > $comment = $2 if defined $2;
> > - $formatted_email =~ s/$address.*$//;
> > + $formatted_email =~ s/\Q$address\E.*$//;
> > $name = $formatted_email;
> > $name = trim($name);
> > $name =~ s/^\"|\"$//g;
>
>
> I can confirm that running
> $ git show HEAD | scripts/checkpatch.pl -
> on a commit that contains a syzbot Reported-by tag does not produce
> the warning anymore.
>
> Joe, do you mind mailing this as patch?

'course not. It's nice you tested it.

I did on the last thousand commits without
apparent change.

$ git log --format=oneline --no-merges -1000 | \
cut -f1 -d" " | \
while read commit ; do \
./scripts/checkpatch.pl --git $commit --types=bad_sign_off --terse ; \
done

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 10:35:55AM -0500, Sowmini Varadhan wrote:
> On (02/14/18 16:28), Dmitry Vyukov wrote:
> > syzbot is probably not seeing this problem. However if you don't add
> > the Reported-by tag to commit, nor provide syz fix tag, it will
> > consider it as "open". One consequence of this is that it is still on
> > our radars. Another consequence is that syzbot will never report bugs
> > in rds_tcp_tune ever again as it thinks that it's the same known bug,
> > so no point in bothering anybody.
>
> understood, I think I saw this in the original syzbot mail as well,
> but I was hesitant to actually add the tag because the fix was
> based on code-inspection only, and I would have felt more comfortable
> about asserting the Reported-by if I'd done a clear-cut before/after
> verification.

I think the point is you have to clear it from syzbot to get it to
even test your patches, even if you are not totally sure your patch
fixes it?

Jason

[Dmitry Vyukov]

Sorry, I failed to parse this sentence. Can you please rephrase it?