Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531299 > unrolled thread

net/dccp: use-after-free in dccp_invalid_packet

Started byAndrey Konovalov <andreyknvl@google.com>
First post2016-11-28 14:30 +0100
Last post2016-11-30 02:40 +0100
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

Contents

  net/dccp: use-after-free in dccp_invalid_packet Andrey Konovalov <andreyknvl@google.com> - 2016-11-28 14:30 +0100
    [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 15:30 +0100
      Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 15:50 +0100
        Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-11-28 16:10 +0100
          Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 16:30 +0100
            Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-11-28 16:40 +0100
      Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-11-28 15:50 +0100
      Re: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet David Miller <davem@davemloft.net> - 2016-11-30 02:40 +0100

#1531299 — net/dccp: use-after-free in dccp_invalid_packet

FromAndrey Konovalov <andreyknvl@google.com>
Date2016-11-28 14:30 +0100
Subjectnet/dccp: use-after-free in dccp_invalid_packet
Message-ID<sIual-2Cp-3@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

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
==================================================================

[toc] | [next] | [standalone]

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

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 15:30 +0100
Subject[PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sIv6p-3fu-13@gated-at.bofh.it>
In reply to#1531299
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 <andreyknvl@google.com>
---
 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

[toc] | [prev] | [next] | [standalone]

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

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 15:50 +0100
SubjectRe: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sIvpL-3m3-23@gated-at.bofh.it>
In reply to#1531348
On Mon, 2016-11-28 at 11:40 -0300, Arnaldo Carvalho de Melo wrote:
> 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 <andreyknvl@google.com>
> 
> Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> 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 <acme@redhat.com>
> 
> 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 <andreyknvl@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> 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 ?

[toc] | [prev] | [next] | [standalone]

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

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-11-28 16:10 +0100
SubjectRe: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sIvJ8-3HN-31@gated-at.bofh.it>
In reply to#1531377
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:
> > 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 <andreyknvl@google.com>
> > 
> > Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > 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 <acme@redhat.com>
> > 
> > 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 <andreyknvl@google.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > 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         }

[toc] | [prev] | [next] | [standalone]

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

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 16:30 +0100
SubjectRe: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sIw2v-3Ow-49@gated-at.bofh.it>
In reply to#1531383
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:
> > On Mon, 2016-11-28 at 11:40 -0300, Arnaldo Carvalho de Melo wrote:
> > > 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 <andreyknvl@google.com>
> > > 
> > > Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > 
> > > 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 <acme@redhat.com>
> > > 
> > > 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 <andreyknvl@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > 
> > > 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.

[toc] | [prev] | [next] | [standalone]

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

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-11-28 16:40 +0100
SubjectRe: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sIwc9-3Rx-3@gated-at.bofh.it>
In reply to#1531394
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:
> > 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:
> > > > 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 <andreyknvl@google.com>
> > > > 
> > > > Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > > 
> > > > 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 <acme@redhat.com>
> > > > 
> > > > 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 <andreyknvl@google.com>
> > > > Cc: Eric Dumazet <edumazet@google.com>
> > > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > > 
> > > > 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

[toc] | [prev] | [next] | [standalone]

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

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-11-28 15:50 +0100
SubjectRe: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sIvpL-3m3-11@gated-at.bofh.it>
In reply to#1531348
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 <andreyknvl@google.com>

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

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 <acme@redhat.com>

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 <andreyknvl@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

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;
 	}

[toc] | [prev] | [next] | [standalone]

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

FromDavid Miller <davem@davemloft.net>
Date2016-11-30 02:40 +0100
SubjectRe: [PATCH net] net/dccp: fix use-after-free in dccp_invalid_packet
Message-ID<sJ22m-7Nz-9@gated-at.bofh.it>
In reply to#1531348
From: Eric Dumazet <eric.dumazet@gmail.com>
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 <andreyknvl@google.com>

Applied and queued up for -stable, thanks Eric.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web