net/dccp: use-after-free in dccp_invalid_packet

8 messages, 4 authors, 2016-11-30 · open the first message on its own page

net/dccp: use-after-free in dccp_invalid_packet

From: Andrey Konovalov <hidden>
Date: 2016-11-28 13:22:43

Hi!

I've got the following error report while running the syzkaller fuzzer.

On commit d8e435f3ab6fea2ea324dce72b51dd7761747523 (Nov 26).

dh->dccph_doff is being accessed (line 731) right after skb was freed
(line 732) in net/dccp/ipv4.c.

A reproducer is attached.

==================================================================
BUG: KASAN: use-after-free in dccp_invalid_packet+0x788/0x800
Read of size 1 at addr ffff880066f0e7c8 by task a.out/3895

page:ffffea00019bc380 count:1 mapcount:0 mapping:          (null)
index:0x0 compound_mapcount: 0
flags: 0x100000000004080(slab|head)
page dumped because: kasan: bad access detected

CPU: 1 PID: 3895 Comm: a.out Not tainted 4.9.0-rc6+ #457
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffff88006cd07758 ffffffff81c73b14 ffff88006cd077e8 ffff880066f0e7c8
 00000000000000fa 00000000000000fb ffff88006cd077d8 ffffffff81637962
 ffff88006cd07810 ffffffff82cc9c90 ffffffff82cabf7c 0000000000000296
Call Trace:
 <IRQ> [   27.672034]  [<ffffffff81c73b14>] dump_stack+0xb3/0x10f
 [<     inline     >] describe_address mm/kasan/report.c:259
 [<ffffffff81637962>] kasan_report_error+0x122/0x560 mm/kasan/report.c:365
 [<     inline     >] kasan_report mm/kasan/report.c:387
 [<ffffffff81637dde>] __asan_report_load1_noabort+0x3e/0x40
mm/kasan/report.c:405
 [<ffffffff839e8158>] dccp_invalid_packet+0x788/0x800 net/dccp/ipv4.c:732
 [<ffffffff839f4fc1>] dccp_v6_rcv+0x21/0x1720 net/dccp/ipv6.c:658
 [<ffffffff83418f13>] ip6_input_finish+0x423/0x15f0 net/ipv6/ip6_input.c:279
 [<     inline     >] NF_HOOK_THRESH ./include/linux/netfilter.h:232
 [<     inline     >] NF_HOOK ./include/linux/netfilter.h:255
 [<ffffffff8341a1ae>] ip6_input+0xce/0x340 net/ipv6/ip6_input.c:322
 [<     inline     >] dst_input ./include/net/dst.h:507
 [<ffffffff834185ae>] ip6_rcv_finish+0x23e/0x780 net/ipv6/ip6_input.c:69
 [<     inline     >] NF_HOOK_THRESH ./include/linux/netfilter.h:232
 [<     inline     >] NF_HOOK ./include/linux/netfilter.h:255
 [<ffffffff8341b4bd>] ipv6_rcv+0x109d/0x1dc0 net/ipv6/ip6_input.c:203
 [<ffffffff82d0805b>] __netif_receive_skb_core+0x187b/0x2a10 net/core/dev.c:4208
 [<ffffffff82d0921a>] __netif_receive_skb+0x2a/0x170 net/core/dev.c:4246
 [<ffffffff82d0bbed>] process_backlog+0xed/0x6e0 net/core/dev.c:4855
 [<     inline     >] napi_poll net/core/dev.c:5156
 [<ffffffff82d0b4cd>] net_rx_action+0x76d/0xda0 net/core/dev.c:5221
 [<ffffffff840f59ef>] __do_softirq+0x23f/0x8e5 kernel/softirq.c:284
 [<ffffffff840f3c1c>] do_softirq_own_stack+0x1c/0x30
arch/x86/entry/entry_64.S:904
 <EOI> [   27.672034]  [<ffffffff81251370>] do_softirq.part.17+0x60/0xa0
 [<     inline     >] do_softirq kernel/softirq.c:176
 [<ffffffff81251466>] __local_bh_enable_ip+0xb6/0xc0 kernel/softirq.c:181
 [<     inline     >] local_bh_enable ./include/linux/bottom_half.h:31
 [<     inline     >] rcu_read_unlock_bh ./include/linux/rcupdate.h:967
 [<ffffffff8340b6c0>] ip6_finish_output2+0xb70/0x1f30 net/ipv6/ip6_output.c:122
 [<ffffffff834152b9>] ip6_finish_output+0x3c9/0x7e0 net/ipv6/ip6_output.c:139
 [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
 [<ffffffff8341588d>] ip6_output+0x1bd/0x6b0 net/ipv6/ip6_output.c:153
 [<     inline     >] dst_output ./include/net/dst.h:501
 [<ffffffff8357a116>] ip6_local_out+0x96/0x170 net/ipv6/output_core.c:170
 [<ffffffff83417b63>] ip6_send_skb+0xa3/0x340 net/ipv6/ip6_output.c:1712
 [<ffffffff83417eb5>] ip6_push_pending_frames+0xb5/0xe0
net/ipv6/ip6_output.c:1732
 [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
 [<ffffffff83489a9e>] rawv6_sendmsg+0x1c4e/0x2c20 net/ipv6/raw.c:920
 [<ffffffff832a1057>] inet_sendmsg+0x317/0x4e0 net/ipv4/af_inet.c:734
 [<     inline     >] sock_sendmsg_nosec net/socket.c:621
 [<ffffffff82c9d76c>] sock_sendmsg+0xcc/0x110 net/socket.c:631
 [<ffffffff82c9f1b7>] ___sys_sendmsg+0x2d7/0x8b0 net/socket.c:1954
 [<ffffffff82ca1888>] __sys_sendmmsg+0x158/0x390 net/socket.c:2044
 [<     inline     >] SYSC_sendmmsg net/socket.c:2075
 [<ffffffff82ca1af5>] SyS_sendmmsg+0x35/0x60 net/socket.c:2070
 [<ffffffff840f2c41>] entry_SYSCALL_64_fastpath+0x1f/0xc2
arch/x86/entry/entry_64.S:209

The buggy address belongs to the object at ffff880066f0e780
 which belongs to the cache kmalloc-512 of size 512
The buggy address ffff880066f0e7c8 is located 72 bytes inside
 of 512-byte region [ffff880066f0e780, ffff880066f0e980)

Freed by task 3895:
 [<ffffffff811aa1b6>] save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57
 [<ffffffff81636a76>] save_stack+0x46/0xd0 mm/kasan/kasan.c:495
 [<     inline     >] set_track mm/kasan/kasan.c:507
 [<ffffffff816372d3>] kasan_slab_free+0x73/0xc0 mm/kasan/kasan.c:571
 [<     inline     >] slab_free_hook mm/slub.c:1352
 [<     inline     >] slab_free_freelist_hook mm/slub.c:1374
 [<     inline     >] slab_free mm/slub.c:2951
 [<ffffffff816337b8>] kfree+0xe8/0x2b0 mm/slub.c:3871
 [<ffffffff82cb6748>] skb_free_head+0x78/0xb0 net/core/skbuff.c:580
 [<ffffffff82cc50fb>] pskb_expand_head+0x28b/0x8f0 net/core/skbuff.c:1244
 [<ffffffff82cc954c>] __pskb_pull_tail+0xcc/0x1190 net/core/skbuff.c:1613
 [<     inline     >] pskb_may_pull ./include/linux/skbuff.h:1966
 [<ffffffff839e80bb>] dccp_invalid_packet+0x6eb/0x800 net/dccp/ipv4.c:731
 [<ffffffff839f4fc1>] dccp_v6_rcv+0x21/0x1720 net/dccp/ipv6.c:658
 [<ffffffff83418f13>] ip6_input_finish+0x423/0x15f0 net/ipv6/ip6_input.c:279
 [<     inline     >] NF_HOOK_THRESH ./include/linux/netfilter.h:232
 [<     inline     >] NF_HOOK ./include/linux/netfilter.h:255
 [<ffffffff8341a1ae>] ip6_input+0xce/0x340 net/ipv6/ip6_input.c:322
 [<     inline     >] dst_input ./include/net/dst.h:507
 [<ffffffff834185ae>] ip6_rcv_finish+0x23e/0x780 net/ipv6/ip6_input.c:69
 [<     inline     >] NF_HOOK_THRESH ./include/linux/netfilter.h:232
 [<     inline     >] NF_HOOK ./include/linux/netfilter.h:255
 [<ffffffff8341b4bd>] ipv6_rcv+0x109d/0x1dc0 net/ipv6/ip6_input.c:203
 [<ffffffff82d0805b>] __netif_receive_skb_core+0x187b/0x2a10 net/core/dev.c:4208
 [<ffffffff82d0921a>] __netif_receive_skb+0x2a/0x170 net/core/dev.c:4246
 [<ffffffff82d0bbed>] process_backlog+0xed/0x6e0 net/core/dev.c:4855
 [<     inline     >] napi_poll net/core/dev.c:5156
 [<ffffffff82d0b4cd>] net_rx_action+0x76d/0xda0 net/core/dev.c:5221
 [<ffffffff840f59ef>] __do_softirq+0x23f/0x8e5 kernel/softirq.c:284

Allocated by task 3895:
 [<ffffffff811aa1b6>] save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57
 [<ffffffff81636a76>] save_stack+0x46/0xd0 mm/kasan/kasan.c:495
 [<     inline     >] set_track mm/kasan/kasan.c:507
 [<ffffffff81636ceb>] kasan_kmalloc+0xab/0xe0 mm/kasan/kasan.c:598
 [<ffffffff81637252>] kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:537
 [<     inline     >] slab_post_alloc_hook mm/slab.h:417
 [<     inline     >] slab_alloc_node mm/slub.c:2708
 [<ffffffff81635fab>] __kmalloc_node_track_caller+0xcb/0x390 mm/slub.c:4270
 [<ffffffff82cb7671>] __kmalloc_reserve.isra.35+0x41/0xe0 net/core/skbuff.c:138
 [<ffffffff82cc4f8c>] pskb_expand_head+0x11c/0x8f0 net/core/skbuff.c:1212
 [<ffffffff82cc954c>] __pskb_pull_tail+0xcc/0x1190 net/core/skbuff.c:1613
 [<     inline     >] pskb_may_pull ./include/linux/skbuff.h:1966
 [<ffffffff839e804b>] dccp_invalid_packet+0x67b/0x800 net/dccp/ipv4.c:708
 [<ffffffff839f4fc1>] dccp_v6_rcv+0x21/0x1720 net/dccp/ipv6.c:658
 [<ffffffff83418f13>] ip6_input_finish+0x423/0x15f0 net/ipv6/ip6_input.c:279
 [<     inline     >] NF_HOOK_THRESH ./include/linux/netfilter.h:232
 [<     inline     >] NF_HOOK ./include/linux/netfilter.h:255
 [<ffffffff8341a1ae>] ip6_input+0xce/0x340 net/ipv6/ip6_input.c:322
 [<     inline     >] dst_input ./include/net/dst.h:507
 [<ffffffff834185ae>] ip6_rcv_finish+0x23e/0x780 net/ipv6/ip6_input.c:69
 [<     inline     >] NF_HOOK_THRESH ./include/linux/netfilter.h:232
 [<     inline     >] NF_HOOK ./include/linux/netfilter.h:255
 [<ffffffff8341b4bd>] ipv6_rcv+0x109d/0x1dc0 net/ipv6/ip6_input.c:203
 [<ffffffff82d0805b>] __netif_receive_skb_core+0x187b/0x2a10 net/core/dev.c:4208
 [<ffffffff82d0921a>] __netif_receive_skb+0x2a/0x170 net/core/dev.c:4246
 [<ffffffff82d0bbed>] process_backlog+0xed/0x6e0 net/core/dev.c:4855
 [<     inline     >] napi_poll net/core/dev.c:5156
 [<ffffffff82d0b4cd>] net_rx_action+0x76d/0xda0 net/core/dev.c:5221
 [<ffffffff840f59ef>] __do_softirq+0x23f/0x8e5 kernel/softirq.c:284

Memory state around the buggy address:
 ffff880066f0e680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff880066f0e700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

ffff880066f0e780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

                                              ^
 ffff880066f0e800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff880066f0e880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

[PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: Eric Dumazet <hidden>
Date: 2016-11-28 14:27:02

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
---
 net/dccp/ipv4.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index b567c8725aea..edbe59d203ef 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c

@@ -700,6 +700,7 @@ int dccp_invalid_packet(struct sk_buff *skb)
 {
 	const struct dccp_hdr *dh;
 	unsigned int cscov;
+	u8 dccph_doff;
 
 	if (skb->pkt_type != PACKET_HOST)
 		return 1;

@@ -721,18 +722,19 @@ int dccp_invalid_packet(struct sk_buff *skb)
 	/*
 	 * If P.Data Offset is too small for packet type, drop packet and return
 	 */
-	if (dh->dccph_doff < dccp_hdr_len(skb) / sizeof(u32)) {
-		DCCP_WARN("P.Data Offset(%u) too small\n", dh->dccph_doff);
+	dccph_doff = dh->dccph_doff;
+	if (dccph_doff < dccp_hdr_len(skb) / sizeof(u32)) {
+		DCCP_WARN("P.Data Offset(%u) too small\n", dccph_doff);
 		return 1;
 	}
 	/*
 	 * If P.Data Offset is too too large for packet, drop packet and return
 	 */
-	if (!pskb_may_pull(skb, dh->dccph_doff * sizeof(u32))) {
-		DCCP_WARN("P.Data Offset(%u) too large\n", dh->dccph_doff);
+	if (!pskb_may_pull(skb, dccph_doff * sizeof(u32))) {
+		DCCP_WARN("P.Data Offset(%u) too large\n", dccph_doff);
 		return 1;
 	}
-
+	dh = dccp_hdr(skb);
 	/*
 	 * If P.type is not Data, Ack, or DataAck and P.X == 0 (the packet
 	 * has short sequence numbers), drop packet and return

Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: 2016-11-28 14:40:21

Em Mon, Nov 28, 2016 at 06:26:49AM -0800, Eric Dumazet escreveu:

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Acked-by: Arnaldo Carvalho de Melo <redacted>

I was about to send exactly this patch, and while looking at it I think
the patch below needs to go in as well, no? To follow the advice of that
Warning line there :-)

From: Arnaldo Carvalho de Melo <redacted>

pskb_may_pull() can reallocate skb->head, so we can't access
iph->frag_off or risk use after free, save it to a variable and us that
later.

Cc: Andrey Konovalov <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Arnaldo Carvalho de Melo <redacted>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 5ddf5cda07f4..9462070561a3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1198,6 +1198,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 	struct iphdr *iph;
 	int proto, tot_len;
 	int nhoff;
+	u16 frag_off;
 	int ihl;
 	int id;

@@ -1213,6 +1214,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 
 	id = ntohs(iph->id);
 	proto = iph->protocol;
+	frag_off = iph->frag_off;
 
 	/* Warning: after this point, iph might be no longer valid */
 	if (unlikely(!pskb_may_pull(skb, ihl)))

@@ -1233,7 +1235,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
 
 		/* fixed ID is invalid if DF bit is not set */
-		if (fixedid && !(iph->frag_off & htons(IP_DF)))
+		if (fixedid && !(frag_off & htons(IP_DF)))
 			goto out;
 	}

Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: Eric Dumazet <hidden>
Date: 2016-11-28 14:47:24

On Mon, 2016-11-28 at 11:40 -0300, Arnaldo Carvalho de Melo wrote:

quoted hunk

Em Mon, Nov 28, 2016 at 06:26:49AM -0800, Eric Dumazet escreveu:

quoted

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Acked-by: Arnaldo Carvalho de Melo <redacted>

I was about to send exactly this patch, and while looking at it I think
the patch below needs to go in as well, no? To follow the advice of that
Warning line there :-)

From: Arnaldo Carvalho de Melo <redacted>

pskb_may_pull() can reallocate skb->head, so we can't access
iph->frag_off or risk use after free, save it to a variable and us that
later.

Cc: Andrey Konovalov <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Arnaldo Carvalho de Melo <redacted>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 5ddf5cda07f4..9462070561a3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1198,6 +1198,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 	struct iphdr *iph;
 	int proto, tot_len;
 	int nhoff;
+	u16 frag_off;
 	int ihl;
 	int id;

@@ -1213,6 +1214,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 
 	id = ntohs(iph->id);
 	proto = iph->protocol;
+	frag_off = iph->frag_off;
 
 	/* Warning: after this point, iph might be no longer valid */
 	if (unlikely(!pskb_may_pull(skb, ihl)))

@@ -1233,7 +1235,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
 
 		/* fixed ID is invalid if DF bit is not set */
-		if (fixedid && !(iph->frag_off & htons(IP_DF)))
+		if (fixedid && !(frag_off & htons(IP_DF)))
 			goto out;
 	}


I do not see why this patch would be needed ?

Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: 2016-11-28 15:06:31

Em Mon, Nov 28, 2016 at 06:47:14AM -0800, Eric Dumazet escreveu:

On Mon, 2016-11-28 at 11:40 -0300, Arnaldo Carvalho de Melo wrote:

quoted

Em Mon, Nov 28, 2016 at 06:26:49AM -0800, Eric Dumazet escreveu:

quoted

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Acked-by: Arnaldo Carvalho de Melo <redacted>

I was about to send exactly this patch, and while looking at it I think
the patch below needs to go in as well, no? To follow the advice of that
Warning line there :-)

From: Arnaldo Carvalho de Melo <redacted>

pskb_may_pull() can reallocate skb->head, so we can't access
iph->frag_off or risk use after free, save it to a variable and us that
later.

Cc: Andrey Konovalov <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Arnaldo Carvalho de Melo <redacted>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 5ddf5cda07f4..9462070561a3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1198,6 +1198,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 	struct iphdr *iph;
 	int proto, tot_len;
 	int nhoff;
+	u16 frag_off;
 	int ihl;
 	int id;

@@ -1213,6 +1214,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 
 	id = ntohs(iph->id);
 	proto = iph->protocol;
+	frag_off = iph->frag_off;
 
 	/* Warning: after this point, iph might be no longer valid */
 	if (unlikely(!pskb_may_pull(skb, ihl)))

@@ -1233,7 +1235,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
 
 		/* fixed ID is invalid if DF bit is not set */
-		if (fixedid && !(iph->frag_off & htons(IP_DF)))
+		if (fixedid && !(frag_off & htons(IP_DF)))
 			goto out;
 	}


I do not see why this patch would be needed ?

Where is iph being reloaded after that pskb_may_pull() and thus at line 1236 we
could use after free? The warning at line 1217?

1209         iph = ip_hdr(skb);
1210         ihl = iph->ihl * 4;
1211         if (ihl < sizeof(*iph))
1212                 goto out;
1213 
1214         id = ntohs(iph->id);
1215         proto = iph->protocol;
1216 
1217         /* Warning: after this point, iph might be no longer valid */
1218         if (unlikely(!pskb_may_pull(skb, ihl)))
1219                 goto out;
1220         __skb_pull(skb, ihl);
1221 
1222         encap = SKB_GSO_CB(skb)->encap_level > 0;
1223         if (encap)
1224                 features &= skb->dev->hw_enc_features;
1225         SKB_GSO_CB(skb)->encap_level += ihl;
1226 
1227         skb_reset_transport_header(skb);
1228 
1229         segs = ERR_PTR(-EPROTONOSUPPORT);
1230 
1231         if (!skb->encapsulation || encap) {
1232                 udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
1233                 fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
1234 
1235                 /* fixed ID is invalid if DF bit is not set */
1236                 if (fixedid && !(iph->frag_off & htons(IP_DF)))
1237                         goto out;
1238         }

Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: Eric Dumazet <hidden>
Date: 2016-11-28 15:21:10

On Mon, 2016-11-28 at 12:05 -0300, Arnaldo Carvalho de Melo wrote:

Em Mon, Nov 28, 2016 at 06:47:14AM -0800, Eric Dumazet escreveu:

quoted

On Mon, 2016-11-28 at 11:40 -0300, Arnaldo Carvalho de Melo wrote:

quoted

Em Mon, Nov 28, 2016 at 06:26:49AM -0800, Eric Dumazet escreveu:

quoted

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Acked-by: Arnaldo Carvalho de Melo <redacted>

I was about to send exactly this patch, and while looking at it I think
the patch below needs to go in as well, no? To follow the advice of that
Warning line there :-)

From: Arnaldo Carvalho de Melo <redacted>

pskb_may_pull() can reallocate skb->head, so we can't access
iph->frag_off or risk use after free, save it to a variable and us that
later.

Cc: Andrey Konovalov <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Arnaldo Carvalho de Melo <redacted>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 5ddf5cda07f4..9462070561a3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1198,6 +1198,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 	struct iphdr *iph;
 	int proto, tot_len;
 	int nhoff;
+	u16 frag_off;
 	int ihl;
 	int id;

@@ -1213,6 +1214,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 
 	id = ntohs(iph->id);
 	proto = iph->protocol;
+	frag_off = iph->frag_off;
 
 	/* Warning: after this point, iph might be no longer valid */
 	if (unlikely(!pskb_may_pull(skb, ihl)))

@@ -1233,7 +1235,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
 
 		/* fixed ID is invalid if DF bit is not set */
-		if (fixedid && !(iph->frag_off & htons(IP_DF)))
+		if (fixedid && !(frag_off & htons(IP_DF)))
 			goto out;
 	}


I do not see why this patch would be needed ?

Where is iph being reloaded after that pskb_may_pull() and thus at line 1236 we
could use after free? The warning at line 1217?

1209         iph = ip_hdr(skb);
1210         ihl = iph->ihl * 4;
1211         if (ihl < sizeof(*iph))
1212                 goto out;
1213 
1214         id = ntohs(iph->id);
1215         proto = iph->protocol;
1216 
1217         /* Warning: after this point, iph might be no longer valid */
1218         if (unlikely(!pskb_may_pull(skb, ihl)))
1219                 goto out;
1220         __skb_pull(skb, ihl);
1221 
1222         encap = SKB_GSO_CB(skb)->encap_level > 0;
1223         if (encap)
1224                 features &= skb->dev->hw_enc_features;
1225         SKB_GSO_CB(skb)->encap_level += ihl;
1226 
1227         skb_reset_transport_header(skb);
1228 
1229         segs = ERR_PTR(-EPROTONOSUPPORT);
1230 
1231         if (!skb->encapsulation || encap) {
1232                 udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
1233                 fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
1234 
1235                 /* fixed ID is invalid if DF bit is not set */
1236                 if (fixedid && !(iph->frag_off & htons(IP_DF)))
1237                         goto out;
1238         }

Arg, I was looking at an old tree.

Please then add

Fixes: cbc53e08a793b ("GSO: Add GSO type for fixed IPv4 ID")

To ease stable backports.

Also, it looks comments are not read, we might kill this one and reload
iph.

( Saving 3 fields is now more expensive than simply reloading iph )

Thanks.

Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: 2016-11-28 15:36:55

Em Mon, Nov 28, 2016 at 07:20:58AM -0800, Eric Dumazet escreveu:

On Mon, 2016-11-28 at 12:05 -0300, Arnaldo Carvalho de Melo wrote:

quoted

Em Mon, Nov 28, 2016 at 06:47:14AM -0800, Eric Dumazet escreveu:

quoted

On Mon, 2016-11-28 at 11:40 -0300, Arnaldo Carvalho de Melo wrote:

quoted

Em Mon, Nov 28, 2016 at 06:26:49AM -0800, Eric Dumazet escreveu:

quoted

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Acked-by: Arnaldo Carvalho de Melo <redacted>

I was about to send exactly this patch, and while looking at it I think
the patch below needs to go in as well, no? To follow the advice of that
Warning line there :-)

From: Arnaldo Carvalho de Melo <redacted>

pskb_may_pull() can reallocate skb->head, so we can't access
iph->frag_off or risk use after free, save it to a variable and us that
later.

Cc: Andrey Konovalov <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Arnaldo Carvalho de Melo <redacted>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 5ddf5cda07f4..9462070561a3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1198,6 +1198,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 	struct iphdr *iph;
 	int proto, tot_len;
 	int nhoff;
+	u16 frag_off;
 	int ihl;
 	int id;

@@ -1213,6 +1214,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 
 	id = ntohs(iph->id);
 	proto = iph->protocol;
+	frag_off = iph->frag_off;
 
 	/* Warning: after this point, iph might be no longer valid */
 	if (unlikely(!pskb_may_pull(skb, ihl)))

@@ -1233,7 +1235,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 		fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
 
 		/* fixed ID is invalid if DF bit is not set */
-		if (fixedid && !(iph->frag_off & htons(IP_DF)))
+		if (fixedid && !(frag_off & htons(IP_DF)))
 			goto out;
 	}


I do not see why this patch would be needed ?

Where is iph being reloaded after that pskb_may_pull() and thus at line 1236 we
could use after free? The warning at line 1217?

1209         iph = ip_hdr(skb);
1210         ihl = iph->ihl * 4;
1211         if (ihl < sizeof(*iph))
1212                 goto out;
1213 
1214         id = ntohs(iph->id);
1215         proto = iph->protocol;
1216 
1217         /* Warning: after this point, iph might be no longer valid */
1218         if (unlikely(!pskb_may_pull(skb, ihl)))
1219                 goto out;
1220         __skb_pull(skb, ihl);
1221 
1222         encap = SKB_GSO_CB(skb)->encap_level > 0;
1223         if (encap)
1224                 features &= skb->dev->hw_enc_features;
1225         SKB_GSO_CB(skb)->encap_level += ihl;
1226 
1227         skb_reset_transport_header(skb);
1228 
1229         segs = ERR_PTR(-EPROTONOSUPPORT);
1230 
1231         if (!skb->encapsulation || encap) {
1232                 udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
1233                 fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
1234 
1235                 /* fixed ID is invalid if DF bit is not set */
1236                 if (fixedid && !(iph->frag_off & htons(IP_DF)))
1237                         goto out;
1238         }

Arg, I was looking at an old tree.

Please then add

Fixes: cbc53e08a793b ("GSO: Add GSO type for fixed IPv4 ID")

To ease stable backports.

Also, it looks comments are not read, we might kill this one and reload
iph.

( Saving 3 fields is now more expensive than simply reloading iph )

Ok, I instead used ip_hdr(skb)->frag_off at that place, as it is the
only use after the pskb_may_pull(), right after that use it will reload
iph in another fashion anyway, sending the patch in another message,
holler if you disagree, i.e. nacking that ack 8-)

- Arnaldo

Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet

From: David Miller <davem@davemloft.net>
Date: 2016-11-30 01:38:24

From: Eric Dumazet <redacted>
Date: Mon, 28 Nov 2016 06:26:49 -0800

From: Eric Dumazet <edumazet@google.com>

pskb_may_pull() can reallocate skb->head, we need to reload dh pointer
in dccp_invalid_packet() or risk use after free.

Bug found by Andrey Konovalov using syzkaller.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>

Applied and queued up for -stable, thanks Eric.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help