netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU
@ 2018-01-10 17:10 Mike Maloney
  2018-01-10 17:21 ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Mike Maloney @ 2018-01-10 17:10 UTC (permalink / raw)
  To: netdev, davem; +Cc: eric.dumazet, Mike Maloney

From: Mike Maloney <maloney@google.com>

The logic in __ip6_append_data() assumes that the MTU is at least large
enough for the headers.  A device's MTU may be adjusted after being
added while sendmsg() is processing data, resulting in
__ip6_append_data() seeing any MTU.  For an mtu smaller than the size of
the fragmentation header, the math results in a negative 'maxfraglen',
which causes problems when refragmenting any previous skb in the
skb_write_queue, leaving it possibly malformed.

Instead sendmsg returns EINVAL when the mtu is calculated to be less
than IPV6_MIN_MTU.

Found by syzkaller:
kernel BUG at ./include/linux/skbuff.h:2064!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 14216 Comm: syz-executor5 Not tainted 4.13.0-rc4+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
task: ffff8801d0b68580 task.stack: ffff8801ac6b8000
RIP: 0010:__skb_pull include/linux/skbuff.h:2064 [inline]
RIP: 0010:__ip6_make_skb+0x18cf/0x1f70 net/ipv6/ip6_output.c:1617
RSP: 0018:ffff8801ac6bf570 EFLAGS: 00010216
RAX: 0000000000010000 RBX: 0000000000000028 RCX: ffffc90003cce000
RDX: 00000000000001b8 RSI: ffffffff839df06f RDI: ffff8801d9478ca0
RBP: ffff8801ac6bf780 R08: ffff8801cc3f1dbc R09: 0000000000000000
R10: ffff8801ac6bf7a0 R11: 43cb4b7b1948a9e7 R12: ffff8801cc3f1dc8
R13: ffff8801cc3f1d40 R14: 0000000000001036 R15: dffffc0000000000
FS:  00007f43d740c700(0000) GS:ffff8801dc100000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f7834984000 CR3: 00000001d79b9000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 ip6_finish_skb include/net/ipv6.h:911 [inline]
 udp_v6_push_pending_frames+0x255/0x390 net/ipv6/udp.c:1093
 udpv6_sendmsg+0x280d/0x31a0 net/ipv6/udp.c:1363
 inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:762
 sock_sendmsg_nosec net/socket.c:633 [inline]
 sock_sendmsg+0xca/0x110 net/socket.c:643
 SYSC_sendto+0x352/0x5a0 net/socket.c:1750
 SyS_sendto+0x40/0x50 net/socket.c:1718
 entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x4512e9
RSP: 002b:00007f43d740bc08 EFLAGS: 00000216 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 00000000007180a8 RCX: 00000000004512e9
RDX: 000000000000002e RSI: 0000000020d08000 RDI: 0000000000000005
RBP: 0000000000000086 R08: 00000000209c1000 R09: 000000000000001c
R10: 0000000000040800 R11: 0000000000000216 R12: 00000000004b9c69
R13: 00000000ffffffff R14: 0000000000000005 R15: 00000000202c2000
Code: 9e 01 fe e9 c5 e8 ff ff e8 7f 9e 01 fe e9 4a ea ff ff 48 89 f7 e8 52 9e 01 fe e9 aa eb ff ff e8 a8 b6 cf fd 0f 0b e8 a1 b6 cf fd <0f> 0b 49 8d 45 78 4d 8d 45 7c 48 89 85 78 fe ff ff 49 8d 85 ba
RIP: __skb_pull include/linux/skbuff.h:2064 [inline] RSP: ffff8801ac6bf570
RIP: __ip6_make_skb+0x18cf/0x1f70 net/ipv6/ip6_output.c:1617 RSP: ffff8801ac6bf570

Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/ipv6/ip6_output.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index f7dd51c42314..f309ce7120d0 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1206,14 +1206,16 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
 	v6_cork->tclass = ipc6->tclass;
 	if (rt->dst.flags & DST_XFRM_TUNNEL)
 		mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
-		      rt->dst.dev->mtu : dst_mtu(&rt->dst);
+		      READ_ONCE(rt->dst.dev->mtu) : dst_mtu(&rt->dst);
 	else
 		mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
-		      rt->dst.dev->mtu : dst_mtu(rt->dst.path);
+		      READ_ONCE(rt->dst.dev->mtu) : dst_mtu(rt->dst.path);
 	if (np->frag_size < mtu) {
 		if (np->frag_size)
 			mtu = np->frag_size;
 	}
+	if (mtu < IPV6_MIN_MTU)
+		return -EINVAL;
 	cork->base.fragsize = mtu;
 	if (dst_allfrag(rt->dst.path))
 		cork->base.flags |= IPCORK_ALLFRAG;
-- 
2.16.0.rc1.238.g530d649a79-goog

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU
  2018-01-10 17:10 [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU Mike Maloney
@ 2018-01-10 17:21 ` Eric Dumazet
  2018-01-10 17:43   ` Mike Maloney
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-01-10 17:21 UTC (permalink / raw)
  To: Mike Maloney, netdev, davem; +Cc: Mike Maloney

On Wed, 2018-01-10 at 12:10 -0500, Mike Maloney wrote:
> From: Mike Maloney <maloney@google.com>
> 
> The logic in __ip6_append_data() assumes that the MTU is at least large
> enough for the headers.  A device's MTU may be adjusted after being
> added while sendmsg() is processing data, resulting in
> __ip6_append_data() seeing any MTU.  For an mtu smaller than the size of
> the fragmentation header, the math results in a negative 'maxfraglen',
> which causes problems when refragmenting any previous skb in the
> skb_write_queue, leaving it possibly malformed.
> 
> Instead sendmsg returns EINVAL when the mtu is calculated to be less
> than IPV6_MIN_MTU.
> 

You forgot your SOB

> Reported-by: syzbot <syzkaller@googlegroups.com>
> ---

Also please add after this '---' marker that your patch depends on my
prior fix ( https://patchwork.ozlabs.org/patch/858234/ 
ipv6: fix possible mem leaks in ipv6_make_skb() )

( Or we leak a dst reference )

We probably should have sent a patch series.

Thanks.

>  net/ipv6/ip6_output.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index f7dd51c42314..f309ce7120d0 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1206,14 +1206,16 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
>  	v6_cork->tclass = ipc6->tclass;
>  	if (rt->dst.flags & DST_XFRM_TUNNEL)
>  		mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
> -		      rt->dst.dev->mtu : dst_mtu(&rt->dst);
> +		      READ_ONCE(rt->dst.dev->mtu) : dst_mtu(&rt->dst);
>  	else
>  		mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
> -		      rt->dst.dev->mtu : dst_mtu(rt->dst.path);
> +		      READ_ONCE(rt->dst.dev->mtu) : dst_mtu(rt->dst.path);
>  	if (np->frag_size < mtu) {
>  		if (np->frag_size)
>  			mtu = np->frag_size;
>  	}
> +	if (mtu < IPV6_MIN_MTU)
> +		return -EINVAL;
>  	cork->base.fragsize = mtu;
>  	if (dst_allfrag(rt->dst.path))
>  		cork->base.flags |= IPCORK_ALLFRAG;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU
  2018-01-10 17:21 ` Eric Dumazet
@ 2018-01-10 17:43   ` Mike Maloney
  0 siblings, 0 replies; 6+ messages in thread
From: Mike Maloney @ 2018-01-10 17:43 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, David Miller, Mike Maloney

D'oh - resending with fixes.  Thanks Eric.

On Wed, Jan 10, 2018 at 12:21 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2018-01-10 at 12:10 -0500, Mike Maloney wrote:
>> From: Mike Maloney <maloney@google.com>
>>
>> The logic in __ip6_append_data() assumes that the MTU is at least large
>> enough for the headers.  A device's MTU may be adjusted after being
>> added while sendmsg() is processing data, resulting in
>> __ip6_append_data() seeing any MTU.  For an mtu smaller than the size of
>> the fragmentation header, the math results in a negative 'maxfraglen',
>> which causes problems when refragmenting any previous skb in the
>> skb_write_queue, leaving it possibly malformed.
>>
>> Instead sendmsg returns EINVAL when the mtu is calculated to be less
>> than IPV6_MIN_MTU.
>>
>
> You forgot your SOB
>
>> Reported-by: syzbot <syzkaller@googlegroups.com>
>> ---
>
> Also please add after this '---' marker that your patch depends on my
> prior fix ( https://patchwork.ozlabs.org/patch/858234/
> ipv6: fix possible mem leaks in ipv6_make_skb() )
>
> ( Or we leak a dst reference )
>
> We probably should have sent a patch series.
>
> Thanks.
>
>>  net/ipv6/ip6_output.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index f7dd51c42314..f309ce7120d0 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -1206,14 +1206,16 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
>>       v6_cork->tclass = ipc6->tclass;
>>       if (rt->dst.flags & DST_XFRM_TUNNEL)
>>               mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
>> -                   rt->dst.dev->mtu : dst_mtu(&rt->dst);
>> +                   READ_ONCE(rt->dst.dev->mtu) : dst_mtu(&rt->dst);
>>       else
>>               mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
>> -                   rt->dst.dev->mtu : dst_mtu(rt->dst.path);
>> +                   READ_ONCE(rt->dst.dev->mtu) : dst_mtu(rt->dst.path);
>>       if (np->frag_size < mtu) {
>>               if (np->frag_size)
>>                       mtu = np->frag_size;
>>       }
>> +     if (mtu < IPV6_MIN_MTU)
>> +             return -EINVAL;
>>       cork->base.fragsize = mtu;
>>       if (dst_allfrag(rt->dst.path))
>>               cork->base.flags |= IPCORK_ALLFRAG;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU
  2018-01-10 17:45 Mike Maloney
  2018-01-10 18:56 ` Eric Dumazet
@ 2018-01-15 18:30 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2018-01-15 18:30 UTC (permalink / raw)
  To: maloneykernel; +Cc: netdev, eric.dumazet, maloney

From: Mike Maloney <maloneykernel@gmail.com>
Date: Wed, 10 Jan 2018 12:45:10 -0500

> From: Mike Maloney <maloney@google.com>
> 
> The logic in __ip6_append_data() assumes that the MTU is at least large
> enough for the headers.  A device's MTU may be adjusted after being
> added while sendmsg() is processing data, resulting in
> __ip6_append_data() seeing any MTU.  For an mtu smaller than the size of
> the fragmentation header, the math results in a negative 'maxfraglen',
> which causes problems when refragmenting any previous skb in the
> skb_write_queue, leaving it possibly malformed.
> 
> Instead sendmsg returns EINVAL when the mtu is calculated to be less
> than IPV6_MIN_MTU.
 ...
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Mike Maloney <maloney@google.com>

Applied and queued up for -stable, thank you.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU
  2018-01-10 17:45 Mike Maloney
@ 2018-01-10 18:56 ` Eric Dumazet
  2018-01-15 18:30 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2018-01-10 18:56 UTC (permalink / raw)
  To: Mike Maloney, netdev, davem; +Cc: Mike Maloney

On Wed, 2018-01-10 at 12:45 -0500, Mike Maloney wrote:
> From: Mike Maloney <maloney@google.com>
> 
> The logic in __ip6_append_data() assumes that the MTU is at least large
> enough for the headers.  A device's MTU may be adjusted after being
> added while sendmsg() is processing data, resulting in
> __ip6_append_data() seeing any MTU.  For an mtu smaller than the size of
> the fragmentation header, the math results in a negative 'maxfraglen',
> which causes problems when refragmenting any previous skb in the
> skb_write_queue, leaving it possibly malformed.
> 
> Instead sendmsg returns EINVAL when the mtu is calculated to be less
> than IPV6_MIN_MTU.
> 

> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Mike Maloney <maloney@google.com>
> 
> 

Reviewed-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU
@ 2018-01-10 17:45 Mike Maloney
  2018-01-10 18:56 ` Eric Dumazet
  2018-01-15 18:30 ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Mike Maloney @ 2018-01-10 17:45 UTC (permalink / raw)
  To: netdev, davem; +Cc: eric.dumazet, Mike Maloney

From: Mike Maloney <maloney@google.com>

The logic in __ip6_append_data() assumes that the MTU is at least large
enough for the headers.  A device's MTU may be adjusted after being
added while sendmsg() is processing data, resulting in
__ip6_append_data() seeing any MTU.  For an mtu smaller than the size of
the fragmentation header, the math results in a negative 'maxfraglen',
which causes problems when refragmenting any previous skb in the
skb_write_queue, leaving it possibly malformed.

Instead sendmsg returns EINVAL when the mtu is calculated to be less
than IPV6_MIN_MTU.

Found by syzkaller:
kernel BUG at ./include/linux/skbuff.h:2064!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 14216 Comm: syz-executor5 Not tainted 4.13.0-rc4+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
task: ffff8801d0b68580 task.stack: ffff8801ac6b8000
RIP: 0010:__skb_pull include/linux/skbuff.h:2064 [inline]
RIP: 0010:__ip6_make_skb+0x18cf/0x1f70 net/ipv6/ip6_output.c:1617
RSP: 0018:ffff8801ac6bf570 EFLAGS: 00010216
RAX: 0000000000010000 RBX: 0000000000000028 RCX: ffffc90003cce000
RDX: 00000000000001b8 RSI: ffffffff839df06f RDI: ffff8801d9478ca0
RBP: ffff8801ac6bf780 R08: ffff8801cc3f1dbc R09: 0000000000000000
R10: ffff8801ac6bf7a0 R11: 43cb4b7b1948a9e7 R12: ffff8801cc3f1dc8
R13: ffff8801cc3f1d40 R14: 0000000000001036 R15: dffffc0000000000
FS:  00007f43d740c700(0000) GS:ffff8801dc100000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f7834984000 CR3: 00000001d79b9000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 ip6_finish_skb include/net/ipv6.h:911 [inline]
 udp_v6_push_pending_frames+0x255/0x390 net/ipv6/udp.c:1093
 udpv6_sendmsg+0x280d/0x31a0 net/ipv6/udp.c:1363
 inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:762
 sock_sendmsg_nosec net/socket.c:633 [inline]
 sock_sendmsg+0xca/0x110 net/socket.c:643
 SYSC_sendto+0x352/0x5a0 net/socket.c:1750
 SyS_sendto+0x40/0x50 net/socket.c:1718
 entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x4512e9
RSP: 002b:00007f43d740bc08 EFLAGS: 00000216 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 00000000007180a8 RCX: 00000000004512e9
RDX: 000000000000002e RSI: 0000000020d08000 RDI: 0000000000000005
RBP: 0000000000000086 R08: 00000000209c1000 R09: 000000000000001c
R10: 0000000000040800 R11: 0000000000000216 R12: 00000000004b9c69
R13: 00000000ffffffff R14: 0000000000000005 R15: 00000000202c2000
Code: 9e 01 fe e9 c5 e8 ff ff e8 7f 9e 01 fe e9 4a ea ff ff 48 89 f7 e8 52 9e 01 fe e9 aa eb ff ff e8 a8 b6 cf fd 0f 0b e8 a1 b6 cf fd <0f> 0b 49 8d 45 78 4d 8d 45 7c 48 89 85 78 fe ff ff 49 8d 85 ba
RIP: __skb_pull include/linux/skbuff.h:2064 [inline] RSP: ffff8801ac6bf570
RIP: __ip6_make_skb+0x18cf/0x1f70 net/ipv6/ip6_output.c:1617 RSP: ffff8801ac6bf570

Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Mike Maloney <maloney@google.com>

---

Depends on eric.dumazet@gmail.com's prior fix or leaks a dst reference.
https://patchwork.ozlabs.org/patch/858234/
(ipv6: fix possible mem leaks in ipv6_make_skb())

 net/ipv6/ip6_output.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index f7dd51c42314..f309ce7120d0 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1206,14 +1206,16 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
 	v6_cork->tclass = ipc6->tclass;
 	if (rt->dst.flags & DST_XFRM_TUNNEL)
 		mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
-		      rt->dst.dev->mtu : dst_mtu(&rt->dst);
+		      READ_ONCE(rt->dst.dev->mtu) : dst_mtu(&rt->dst);
 	else
 		mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ?
-		      rt->dst.dev->mtu : dst_mtu(rt->dst.path);
+		      READ_ONCE(rt->dst.dev->mtu) : dst_mtu(rt->dst.path);
 	if (np->frag_size < mtu) {
 		if (np->frag_size)
 			mtu = np->frag_size;
 	}
+	if (mtu < IPV6_MIN_MTU)
+		return -EINVAL;
 	cork->base.fragsize = mtu;
 	if (dst_allfrag(rt->dst.path))
 		cork->base.flags |= IPCORK_ALLFRAG;
-- 
2.16.0.rc1.238.g530d649a79-goog

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-01-15 18:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10 17:10 [PATCH net] ipv6: fix udpv6 sendmsg crash caused by too small MTU Mike Maloney
2018-01-10 17:21 ` Eric Dumazet
2018-01-10 17:43   ` Mike Maloney
2018-01-10 17:45 Mike Maloney
2018-01-10 18:56 ` Eric Dumazet
2018-01-15 18:30 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).