net/llc: BUG in llc_sap_state_process/skb_set_owner_r

Hi,

I've got the following error report while fuzzing the kernel with syzkaller.

On commit 926af6273fc683cd98cd0ce7bf0d04a02eed6742.

A reproducer and .config are attached

kernel BUG at ./include/linux/skbuff.h:2389!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 0 PID: 9315 Comm: syz-executor2 Not tainted 4.10.0-rc7+ #126
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88006861c480 task.stack: ffff88006a988000
RIP: 0010:skb_set_owner_r include/linux/skbuff.h:2389 [inline]
RIP: 0010:__sock_queue_rcv_skb+0x8c0/0xda0 net/core/sock.c:425
RSP: 0018:ffff88003ec06b58 EFLAGS: 00010206
RAX: ffff88006861c480 RBX: ffff8800371c2568 RCX: 0000000000000000
RDX: 0000000000000100 RSI: 1ffff10006ba08ab RDI: ffff880035d04560
RBP: ffff88003ec06dc0 R08: 0000000000000002 R09: 0000000000000001
R10: 0000000000000000 R11: dffffc0000000000 R12: ffff880035d04540
R13: ffff88003ec06d98 R14: ffff8800371c2590 R15: ffff880035d045a0
FS:  00007fa8005ac700(0000) GS:ffff88003ec00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004a6f68 CR3: 0000000038e25000 CR4: 00000000000006f0
Call Trace:
 <IRQ>
 sock_queue_rcv_skb+0x3a/0x50 net/core/sock.c:451
 llc_sap_state_process+0x3e3/0x4e0 net/llc/llc_sap.c:220
 llc_sap_rcv net/llc/llc_sap.c:294 [inline]
 llc_sap_handler+0x695/0x1320 net/llc/llc_sap.c:434
 llc_rcv+0x6da/0xed0 net/llc/llc_input.c:208
 __netif_receive_skb_core+0x1ae5/0x3400 net/core/dev.c:4190
 __netif_receive_skb+0x2a/0x170 net/core/dev.c:4228
 process_backlog+0xe5/0x6c0 net/core/dev.c:4839
 napi_poll net/core/dev.c:5202 [inline]
 net_rx_action+0xe70/0x1900 net/core/dev.c:5267
 __do_softirq+0x2fb/0xb7d kernel/softirq.c:284
 do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:902
 </IRQ>
 do_softirq.part.17+0x1e8/0x230 kernel/softirq.c:328
 do_softirq kernel/softirq.c:176 [inline]
 __local_bh_enable_ip+0x1f2/0x200 kernel/softirq.c:181
 local_bh_enable include/linux/bottom_half.h:31 [inline]
 rcu_read_unlock_bh include/linux/rcupdate.h:971 [inline]
 __dev_queue_xmit+0xd87/0x2860 net/core/dev.c:3399
 dev_queue_xmit+0x17/0x20 net/core/dev.c:3405
 llc_build_and_send_ui_pkt+0x240/0x330 net/llc/llc_output.c:74
 llc_ui_sendmsg+0x98d/0x1430 net/llc/af_llc.c:928
 sock_sendmsg_nosec net/socket.c:635 [inline]
 sock_sendmsg+0xca/0x110 net/socket.c:645
 ___sys_sendmsg+0x9d2/0xae0 net/socket.c:1985
 __sys_sendmsg+0x138/0x320 net/socket.c:2019
 SYSC_sendmsg net/socket.c:2030 [inline]
 SyS_sendmsg+0x2d/0x50 net/socket.c:2026
 entry_SYSCALL_64_fastpath+0x1f/0xc2
RIP: 0033:0x4458b9
RSP: 002b:00007fa8005abb58 EFLAGS: 00000286 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00000000004458b9
RDX: 0000000000040880 RSI: 0000000020003000 RDI: 0000000000000006
RBP: 00000000006e1b00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000286 R12: 0000000000708000
R13: 0000000000000082 R14: 0000000020000000 R15: 0000000000000000
Code: 4b 50 fe e9 b1 f8 ff ff e8 3e 4a 50 fe e9 78 f8 ff ff e8 34 4a
50 fe e9 6d f9 ff ff e8 2a 4a 50 fe e9 93 f9 ff ff e8 20 0a 26 fe <0f>
0b e8 19 0a 26 fe be 3c 01 00 00 48 c7 c7 e0 e9 22 85 e8 b8
RIP: skb_set_owner_r include/linux/skbuff.h:2389 [inline] RSP: ffff88003ec06b58
RIP: __sock_queue_rcv_skb+0x8c0/0xda0 net/core/sock.c:425 RSP: ffff88003ec06b58
---[ end trace 58af2d02ad7f84f0 ]---
Kernel panic - not syncing: Fatal exception in interrupt
Dumping ftrace buffer:
   (ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..

On Sun, Feb 12, 2017 at 8:44 AM, Andrey Konovalov [off-list ref] wrote:

Hi,

I've got the following error report while fuzzing the kernel with syzkaller.

On commit 926af6273fc683cd98cd0ce7bf0d04a02eed6742.

A reproducer and .config are attached

Thanks for the report.

llc sets skb->sk   without corresponding skb->destructor.

This is considered invalid by our current standards.

As I added the sanity check in skb_destructor() back in linux-3.12
(!!!), I will send the corresponding LLC fix.

( commit 376c7311bdb6efea3322310333576a04d73fbe4c )

From: Eric Dumazet <edumazet@google.com>

It seems nobody used LLC since linux-3.12.

Fortunately fuzzers like syzkaller still know how to run this code,
otherwise it would be no fun.

Setting skb->sk without skb->destructor leads to all kinds of
bugs, we now prefer to be very strict about it.

Ideally here we would use skb_set_owner() but this helper does not exist yet,
only CAN seems to have a private helper for that.

Fixes: 376c7311bdb6 ("net: add a temporary sanity check in skb_orphan()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
---
 net/llc/llc_conn.c |    3 +++
 net/llc/llc_sap.c  |    3 +++
 2 files changed, 6 insertions(+)

diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 3e821daf9dd4a2fbf00550591e92b153efd4a73a..8bc5a1bd2d453542df31506f543feb64b64cdd96 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c

@@ -821,7 +821,10 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
 		 * another trick required to cope with how the PROCOM state
 		 * machine works. -acme
 		 */
+		skb_orphan(skb);
+		sock_hold(sk);
 		skb->sk = sk;
+		skb->destructor = sock_efree;
 	}
 	if (!sock_owned_by_user(sk))
 		llc_conn_rcv(sk, skb);

diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
index d0e1e804ebd73dcebcf2f930b921233a49b0f454..5404d0d195cc581613e356b75bd70321e617673e 100644
--- a/net/llc/llc_sap.c
+++ b/net/llc/llc_sap.c

@@ -290,7 +290,10 @@ static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb,
 
 	ev->type   = LLC_SAP_EV_TYPE_PDU;
 	ev->reason = 0;
+	skb_orphan(skb);
+	sock_hold(sk);
 	skb->sk = sk;
+	skb->destructor = sock_efree;
 	llc_sap_state_process(sap, skb);
 }
 

From: Eric Dumazet <redacted>
Date: Sun, 12 Feb 2017 14:03:52 -0800

From: Eric Dumazet <edumazet@google.com>

It seems nobody used LLC since linux-3.12.

Fortunately fuzzers like syzkaller still know how to run this code,
otherwise it would be no fun.

Setting skb->sk without skb->destructor leads to all kinds of
bugs, we now prefer to be very strict about it.

Ideally here we would use skb_set_owner() but this helper does not exist yet,
only CAN seems to have a private helper for that.

Fixes: 376c7311bdb6 ("net: add a temporary sanity check in skb_orphan()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Applied and queued up for -stable, thanks Eric.