diff mbox

net/packet: use-after-free in packet_rcv_fanout

Message ID 1486697003.7793.121.camel@edumazet-glaptop3.roam.corp.google.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Feb. 10, 2017, 3:23 a.m. UTC 
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:

> More likely the bug is in fanout_add(), with a buggy sequence in error
> case, and not correct locking.
> 
> kfree(po->rollover);
> po->rollover = NULL;
> 
> Two cpus entering fanout_add() (using the same af_packet socket,
> syzkaller courtesy...) might both see po->fanout being NULL.
> 
> Then they grab the mutex.  Too late...

Patch could be :

 net/packet/af_packet.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Cong Wang Feb. 10, 2017, 5:49 p.m. UTC | #1 
On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
>
>> More likely the bug is in fanout_add(), with a buggy sequence in error
>> case, and not correct locking.
>>
>> kfree(po->rollover);
>> po->rollover = NULL;
>>
>> Two cpus entering fanout_add() (using the same af_packet socket,
>> syzkaller courtesy...) might both see po->fanout being NULL.
>>
>> Then they grab the mutex.  Too late...
>
> Patch could be :
>

For me, clearly the data structure that use-after-free'd is struct sock
rather than struct packet_rollover.
Eric Dumazet Feb. 10, 2017, 5:59 p.m. UTC | #2 
On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
> On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
> >
> >> More likely the bug is in fanout_add(), with a buggy sequence in error
> >> case, and not correct locking.
> >>
> >> kfree(po->rollover);
> >> po->rollover = NULL;
> >>
> >> Two cpus entering fanout_add() (using the same af_packet socket,
> >> syzkaller courtesy...) might both see po->fanout being NULL.
> >>
> >> Then they grab the mutex.  Too late...
> >
> > Patch could be :
> >
> 
> For me, clearly the data structure that use-after-free'd is struct sock
> rather than struct packet_rollover.

Fine. But your patch makes absolutely no sense.
Cong Wang Feb. 10, 2017, 6:02 p.m. UTC | #3 
On Fri, Feb 10, 2017 at 9:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
>> On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
>> >
>> >> More likely the bug is in fanout_add(), with a buggy sequence in error
>> >> case, and not correct locking.
>> >>
>> >> kfree(po->rollover);
>> >> po->rollover = NULL;
>> >>
>> >> Two cpus entering fanout_add() (using the same af_packet socket,
>> >> syzkaller courtesy...) might both see po->fanout being NULL.
>> >>
>> >> Then they grab the mutex.  Too late...
>> >
>> > Patch could be :
>> >
>>
>> For me, clearly the data structure that use-after-free'd is struct sock
>> rather than struct packet_rollover.
>
> Fine. But your patch makes absolutely no sense.

I don't have to give a 100% correct patch to prove my explanation
of the crash. At least it makes more sense than yours...
Eric Dumazet Feb. 10, 2017, 6:02 p.m. UTC | #4 
On Fri, 2017-02-10 at 09:59 -0800, Eric Dumazet wrote:
> On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
> > On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
> > >
> > >> More likely the bug is in fanout_add(), with a buggy sequence in error
> > >> case, and not correct locking.
> > >>
> > >> kfree(po->rollover);
> > >> po->rollover = NULL;
> > >>
> > >> Two cpus entering fanout_add() (using the same af_packet socket,
> > >> syzkaller courtesy...) might both see po->fanout being NULL.
> > >>
> > >> Then they grab the mutex.  Too late...
> > >
> > > Patch could be :
> > >
> > 
> > For me, clearly the data structure that use-after-free'd is struct sock
> > rather than struct packet_rollover.
> 
> Fine. But your patch makes absolutely no sense.

At least, Anoob patch is making a step into the right direction ;)

https://patchwork.ozlabs.org/patch/726532/
Eric Dumazet Feb. 10, 2017, 6:15 p.m. UTC | #5 
On Fri, 2017-02-10 at 10:02 -0800, Cong Wang wrote:

> I don't have to give a 100% correct patch to prove my explanation
> of the crash. At least it makes more sense than yours...

I will submit it regardless of what you think.

It solves _another_ issue, one of of 10 in af_packet.c
Cong Wang Feb. 10, 2017, 6:34 p.m. UTC | #6 
On Fri, Feb 10, 2017 at 10:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2017-02-10 at 09:59 -0800, Eric Dumazet wrote:
>> On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
>> > On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > > On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
>> > >
>> > >> More likely the bug is in fanout_add(), with a buggy sequence in error
>> > >> case, and not correct locking.
>> > >>
>> > >> kfree(po->rollover);
>> > >> po->rollover = NULL;
>> > >>
>> > >> Two cpus entering fanout_add() (using the same af_packet socket,
>> > >> syzkaller courtesy...) might both see po->fanout being NULL.
>> > >>
>> > >> Then they grab the mutex.  Too late...
>> > >
>> > > Patch could be :
>> > >
>> >
>> > For me, clearly the data structure that use-after-free'd is struct sock
>> > rather than struct packet_rollover.
>>
>> Fine. But your patch makes absolutely no sense.
>
> At least, Anoob patch is making a step into the right direction ;)
>
> https://patchwork.ozlabs.org/patch/726532/
>

Yeah, but still looks like a different one with the one Dmitry reported.
Sowmini Varadhan Feb. 13, 2017, 1:42 a.m. UTC | #7 
On (02/10/17 10:02), Eric Dumazet wrote:
> At least, Anoob patch is making a step into the right direction ;)
> https://patchwork.ozlabs.org/patch/726532/

I've not been able to reproduce Dmitry's panic (though I did not try
very hard either) but there's a call to fanout_release from packet_release
before the synchronize_net() - I wonder if this could end up kfree'ing f
when there are threads in the middle of dev_queue_xmit_nit().

--Sowmini
Dmitry Vyukov Feb. 13, 2017, 3:17 p.m. UTC | #8 
On Mon, Feb 13, 2017 at 2:42 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (02/10/17 10:02), Eric Dumazet wrote:
>> At least, Anoob patch is making a step into the right direction ;)
>> https://patchwork.ozlabs.org/patch/726532/
>
> I've not been able to reproduce Dmitry's panic (though I did not try
> very hard either) but there's a call to fanout_release from packet_release
> before the synchronize_net() - I wonder if this could end up kfree'ing f
> when there are threads in the middle of dev_queue_xmit_nit().


Another similar one:

BUG: KASAN: use-after-free in fanout_demux_rollover+0x681/0x6b0
net/packet/af_packet.c:1387 at addr ffff8801cf083df0
Read of size 8 by task syz-executor1/21190
CPU: 1 PID: 21190 Comm: syz-executor1 Not tainted 4.9.0 #7
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
 ffff8801c2616850 ffffffff8234ce1f ffffffff00000001 1ffff100384c2c9d
 ffffed00384c2c95 0000000041b58ab3 ffffffff84b38258 ffffffff8234cb31
 ffffe8ffffd2b3b8 0000000000000000 ffffffff84aef298 ffffffff8155e0a0
Call Trace:
 [<ffffffff8234ce1f>] __dump_stack lib/dump_stack.c:15 [inline]
 [<ffffffff8234ce1f>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
 [<ffffffff819e242c>] kasan_object_err+0x1c/0x70 mm/kasan/report.c:162
 [<ffffffff819e26c5>] print_address_description mm/kasan/report.c:200 [inline]
 [<ffffffff819e26c5>] kasan_report_error mm/kasan/report.c:289 [inline]
 [<ffffffff819e26c5>] kasan_report.part.2+0x1e5/0x4b0 mm/kasan/report.c:311
 [<ffffffff819e2a49>] kasan_report mm/kasan/report.c:332 [inline]
 [<ffffffff819e2a49>] __asan_report_load8_noabort+0x29/0x30
mm/kasan/report.c:332
 [<ffffffff83acaf61>] fanout_demux_rollover+0x681/0x6b0
net/packet/af_packet.c:1387
 [<ffffffff83ad35fe>] packet_rcv_fanout+0x56e/0x800 net/packet/af_packet.c:1473
 [<ffffffff8357b9ea>] dev_queue_xmit_nit+0x73a/0xa90 net/core/dev.c:1904
 [<ffffffff8358d78b>] xmit_one net/core/dev.c:2910 [inline]
 [<ffffffff8358d78b>] dev_hard_start_xmit+0x16b/0xab0 net/core/dev.c:2930
 [<ffffffff8358fac4>] __dev_queue_xmit+0x16e4/0x1e70 net/core/dev.c:3391
 [<ffffffff83590267>] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424
 [<ffffffff8373771e>] neigh_hh_output include/net/neighbour.h:468 [inline]
 [<ffffffff8373771e>] dst_neigh_output include/net/dst.h:462 [inline]
 [<ffffffff8373771e>] ip_finish_output2+0xf0e/0x1520 net/ipv4/ip_output.c:224
 [<ffffffff8373b659>] ip_finish_output+0x909/0xc60 net/ipv4/ip_output.c:305
 [<ffffffff837402b7>] NF_HOOK_COND include/linux/netfilter.h:246 [inline]
 [<ffffffff837402b7>] ip_output+0x1f7/0x860 net/ipv4/ip_output.c:379
 [<ffffffff8373c2c5>] dst_output include/net/dst.h:501 [inline]
 [<ffffffff8373c2c5>] ip_local_out+0x95/0x170 net/ipv4/ip_output.c:123
 [<ffffffff8374348c>] ip_send_skb+0x3c/0xc0 net/ipv4/ip_output.c:1461
 [<ffffffff83743574>] ip_push_pending_frames+0x64/0x80 net/ipv4/ip_output.c:1481
 [<ffffffff838102b0>] raw_sendmsg+0x19c0/0x2070 net/ipv4/raw.c:650
 [<ffffffff83857370>] inet_sendmsg+0x380/0x580 net/ipv4/af_inet.c:734
 [<ffffffff834f6aaa>] sock_sendmsg_nosec net/socket.c:621 [inline]
 [<ffffffff834f6aaa>] sock_sendmsg+0xca/0x110 net/socket.c:631
 [<ffffffff834f82f0>] SYSC_sendto+0x660/0x810 net/socket.c:1656
 [<ffffffff834fb260>] SyS_sendto+0x40/0x50 net/socket.c:1624
 [<ffffffff843778c1>] entry_SYSCALL_64_fastpath+0x1f/0xc2
Object at ffff8801cf0836c0, in cache kmalloc-2048 size: 2048
Allocated:
PID = 21136
 [  142.233815] [<ffffffff8129c696>] save_stack_trace+0x16/0x20
arch/x86/kernel/stacktrace.c:57
 [  142.241057] [<ffffffff819e16c3>] save_stack+0x43/0xd0 mm/kasan/kasan.c:495
 [  142.247780] [<ffffffff819e194a>] set_track mm/kasan/kasan.c:507 [inline]
 [  142.247780] [<ffffffff819e194a>] kasan_kmalloc+0xaa/0xd0
mm/kasan/kasan.c:598
 [  142.254778] [<ffffffff819dcf2c>] __do_kmalloc mm/slab.c:3737 [inline]
 [  142.254778] [<ffffffff819dcf2c>] __kmalloc+0x12c/0x690 mm/slab.c:3746
 [  142.261585] [<ffffffff83501881>] kmalloc include/linux/slab.h:497 [inline]
 [  142.261585] [<ffffffff83501881>] sk_prot_alloc+0x101/0x2a0
net/core/sock.c:1333
 [  142.268791] [<ffffffff8350a20c>] sk_alloc+0x8c/0x460 net/core/sock.c:1389
 [  142.275425] [<ffffffff83ac9673>] packet_create+0x163/0xb00
net/packet/af_packet.c:3181
 [  142.282590] [<ffffffff834f9f24>] __sock_create+0x4e4/0x870 net/socket.c:1168
 [  142.289754] [<ffffffff834fa4e9>] sock_create net/socket.c:1208 [inline]
 [  142.289754] [<ffffffff834fa4e9>] SYSC_socket net/socket.c:1238 [inline]
 [  142.289754] [<ffffffff834fa4e9>] SyS_socket+0xf9/0x230 net/socket.c:1218
 [  142.296558] [<ffffffff843778c1>] entry_SYSCALL_64_fastpath+0x1f/0xc2
Freed:
PID = 21163
 [  142.309281] [<ffffffff8129c696>] save_stack_trace+0x16/0x20
arch/x86/kernel/stacktrace.c:57
 [  142.316556] [<ffffffff819e16c3>] save_stack+0x43/0xd0 mm/kasan/kasan.c:495
 [  142.323282] [<ffffffff819e1fbf>] set_track mm/kasan/kasan.c:507 [inline]
 [  142.323282] [<ffffffff819e1fbf>] kasan_slab_free+0x6f/0xb0
mm/kasan/kasan.c:571
 [  142.330450] [<ffffffff819dfeb3>] __cache_free mm/slab.c:3515 [inline]
 [  142.330450] [<ffffffff819dfeb3>] kfree+0xd3/0x250 mm/slab.c:3832
 [  142.336840] [<ffffffff83505615>] sk_prot_free net/core/sock.c:1372 [inline]
 [  142.336840] [<ffffffff83505615>] __sk_destruct+0x5a5/0x6a0
net/core/sock.c:1445
 [  142.344013] [<ffffffff8350fa77>] sk_destruct+0x47/0x80 net/core/sock.c:1453
 [  142.350819] [<ffffffff8350fb07>] __sk_free+0x57/0x230 net/core/sock.c:1461
 [  142.357535] [<ffffffff8350fd03>] sk_free+0x23/0x30 net/core/sock.c:1472
 [  142.364024] [<ffffffff83ace53c>] sock_put include/net/sock.h:1591 [inline]
 [  142.364024] [<ffffffff83ace53c>] packet_release+0x7ac/0x970
net/packet/af_packet.c:3021
 [  142.371267] [<ffffffff834f258d>] sock_release+0x8d/0x1e0 net/socket.c:585
 [  142.378247] [<ffffffff834f26f6>] sock_close+0x16/0x20 net/socket.c:1032
 [  142.384968] [<ffffffff81a34772>] __fput+0x332/0x7f0 fs/file_table.c:208
 [  142.391512] [<ffffffff81a34cb5>] ____fput+0x15/0x20 fs/file_table.c:244
 [  142.398088] [<ffffffff814a58ca>] task_work_run+0x18a/0x260
kernel/task_work.c:116
 [  142.405266] [<ffffffff8142ff8f>] exit_task_work
include/linux/task_work.h:21 [inline]
 [  142.405266] [<ffffffff8142ff8f>] do_exit+0x18ef/0x2890 kernel/exit.c:830
 [  142.412090] [<ffffffff81435989>] do_group_exit+0x149/0x420 kernel/exit.c:934
 [  142.419243] [<ffffffff81465120>] get_signal+0x7e0/0x1820
kernel/signal.c:2307
 [  142.426220] [<ffffffff812665a2>] do_signal+0xd2/0x2120
arch/x86/kernel/signal.c:807
 [  142.433057] [<ffffffff81007900>] exit_to_usermode_loop+0x200/0x2a0
arch/x86/entry/common.c:156
 [  142.440908] [<ffffffff81009413>] prepare_exit_to_usermode
arch/x86/entry/common.c:190 [inline]
 [  142.440908] [<ffffffff81009413>]
syscall_return_slowpath+0x4d3/0x570 arch/x86/entry/common.c:259
 [  142.448950] [<ffffffff84377962>] entry_SYSCALL_64_fastpath+0xc0/0xc2
Memory state around the buggy address:
 ffff8801cf083c80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff8801cf083d00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff8801cf083d80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                                             ^
 ffff8801cf083e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff8801cf083e80: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
==================================================================
Cong Wang Feb. 17, 2017, 7:27 p.m. UTC | #9 
On Mon, Feb 13, 2017 at 7:17 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> Another similar one:
>

The other possibility is:  __fanout_link() is called twice on the same
packet sock
for some reason, but __fanout_unlink() only unlinks the first one, which led to
this use-after-free. However, the po->running and po->fanout seem enough
to guarantee this should not happen. I still want to point this out in case I
miss anything here so that other people could figure it out.
diff mbox

Patch

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index d56ee46b11fc9524e457e5fe8adf10c105a66ab6..11725a350f6953d077f754c10e9f52e48924d780 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1657,7 +1657,6 @@  static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
 		atomic_long_set(&po->rollover->num_failed, 0);
 	}
 
-	mutex_lock(&fanout_mutex);
 	match = NULL;
 	list_for_each_entry(f, &fanout_list, list) {
 		if (f->id == id &&
@@ -1704,7 +1703,6 @@  static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
 		}
 	}
 out:
-	mutex_unlock(&fanout_mutex);
 	if (err) {
 		kfree(po->rollover);
 		po->rollover = NULL;
@@ -3698,7 +3696,10 @@  packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
 		if (copy_from_user(&val, optval, sizeof(val)))
 			return -EFAULT;
 
-		return fanout_add(sk, val & 0xffff, val >> 16);
+		mutex_lock(&fanout_mutex);
+		ret = fanout_add(sk, val & 0xffff, val >> 16);
+		mutex_unlock(&fanout_mutex);
+		return ret;
 	}
 	case PACKET_FANOUT_DATA:
 	{