[PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy_receive()

[PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy_receive()

[Eric Dumazet]

Apparently a refactoring patch brought a bug, that was caught
by syzbot [1]

Original code was correct, do not try to be smarter than the
compiler :/

[1]
BUG: KASAN: slab-out-of-bounds in tcp_zerocopy_receive net/ipv4/tcp.c:1807 [inline]
BUG: KASAN: slab-out-of-bounds in do_tcp_getsockopt.isra.0+0x2c6c/0x3120 net/ipv4/tcp.c:3654
Read of size 4 at addr ffff8880943cf188 by task syz-executor.2/17508

CPU: 0 PID: 17508 Comm: syz-executor.2 Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x172/0x1f0 lib/dump_stack.c:113
 print_address_description.cold+0xd4/0x306 mm/kasan/report.c:351
 __kasan_report.cold+0x1b/0x36 mm/kasan/report.c:482
 kasan_report+0x12/0x17 mm/kasan/common.c:618
 __asan_report_load4_noabort+0x14/0x20 mm/kasan/generic_report.c:131
 tcp_zerocopy_receive net/ipv4/tcp.c:1807 [inline]
 do_tcp_getsockopt.isra.0+0x2c6c/0x3120 net/ipv4/tcp.c:3654
 tcp_getsockopt+0xbf/0xe0 net/ipv4/tcp.c:3680
 sock_common_getsockopt+0x94/0xd0 net/core/sock.c:3098
 __sys_getsockopt+0x16d/0x310 net/socket.c:2129
 __do_sys_getsockopt net/socket.c:2144 [inline]
 __se_sys_getsockopt net/socket.c:2141 [inline]
 __x64_sys_getsockopt+0xbe/0x150 net/socket.c:2141
 do_syscall_64+0xfd/0x6a0 arch/x86/entry/common.c:296

Fixes: d8e18a516f8f ("net: Use skb accessors in network core")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/ipv4/tcp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 79c325a07ba5dc7cfad0a846d1f03bf1787f840b..f98a1882e537dca0102e829cb349be50302d83ab 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1798,13 +1798,11 @@ static int tcp_zerocopy_receive(struct sock *sk,
 		}
 		if (skb_frag_size(frags) != PAGE_SIZE || skb_frag_off(frags)) {
 			int remaining = zc->recv_skip_hint;
-			int size = skb_frag_size(frags);
 
-			while (remaining && (size != PAGE_SIZE ||
+			while (remaining && (skb_frag_size(frags) != PAGE_SIZE ||
 					     skb_frag_off(frags))) {
-				remaining -= size;
+				remaining -= skb_frag_size(frags);
 				frags++;
-				size = skb_frag_size(frags);
 			}
 			zc->recv_skip_hint -= remaining;
 			break;
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy_receive()

[Matthew Wilcox]

On Wed, Oct 02, 2019 at 08:19:59PM -0700, Eric Dumazet wrote:
> Apparently a refactoring patch brought a bug, that was caught
> by syzbot [1]

That wasn't refactoring.  As you know (because we talked about it at
LSFMM), this is an enabling patch for supporting hch's work to fix
get_user_pages().

> Original code was correct, do not try to be smarter than the
> compiler :/

That wasn't an attempt to be smarter than the compiler.  I was trying
to keep the line length below 80 columns.  Which you probably now see
that you haven't done.

I must have a blind spot here.  I can't see the difference between the
two versions.

> +++ b/net/ipv4/tcp.c
> @@ -1798,13 +1798,11 @@ static int tcp_zerocopy_receive(struct sock *sk,
>  		}
>  		if (skb_frag_size(frags) != PAGE_SIZE || skb_frag_off(frags)) {
>  			int remaining = zc->recv_skip_hint;
> -			int size = skb_frag_size(frags);
>  
> -			while (remaining && (size != PAGE_SIZE ||
> +			while (remaining && (skb_frag_size(frags) != PAGE_SIZE ||
>  					     skb_frag_off(frags))) {
> -				remaining -= size;
> +				remaining -= skb_frag_size(frags);
>  				frags++;
> -				size = skb_frag_size(frags);
>  			}
>  			zc->recv_skip_hint -= remaining;
>  			break;
> -- 
> 2.23.0.581.g78d2f28ef7-goog
> 

Re: [PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy_receive()

[Eric Dumazet]

On 10/3/19 2:46 AM, Matthew Wilcox wrote:
> On Wed, Oct 02, 2019 at 08:19:59PM -0700, Eric Dumazet wrote:
>> Apparently a refactoring patch brought a bug, that was caught
>> by syzbot [1]
> 
> That wasn't refactoring.  As you know (because we talked about it at
> LSFMM), this is an enabling patch for supporting hch's work to fix
> get_user_pages().

Changing frags->size by skb_frag_size(frags) is what I call refactoring.

This was claimed in the changelog of your patch :

<quote>
    net: Use skb accessors in network core
    
    In preparation for unifying the skb_frag and bio_vec, use the fine
    accessors which already exist and use skb_frag_t instead of
    struct skb_frag_struct.
</quote>

I guess David trusted you enough and did not checked that you made other changes.

> 
>> Original code was correct, do not try to be smarter than the
>> compiler :/
> 
> That wasn't an attempt to be smarter than the compiler.  I was trying
> to keep the line length below 80 columns.  Which you probably now see
> that you haven't done.

Seriously we do not care of the 80 column rule in this particular function,
we care about code correctness first.

We do not mix fixes and cleanups in the same patch.

> 
> I must have a blind spot here.  I can't see the difference between the
> two versions.

No worries.

The major difference is that with your code, when @remaining reaches zero,
it fetches an extra "size = skb_frag_size(frags);" at line 1807
as the syzbot report correctly points.

MAX_SKB_FRAGS is 17 on x86, meaning struct skb_shared_info frags[] array
has 17 elements.

Accessing the 18th element can trigger a page fault, if struct skb_shared_info
sits exactly at the end of a page.

while (remaining && (size != PAGE_SIZE ||
		     skb_frag_off(frags))) {

	remaining -= size; // when this reaches 0

	frags++; // this points to the next fragment in the array, not initialized.

	size = skb_frag_size(frags); // Crash here if crossing a page and next  page is unmapped.
}

Original code from Soheil was correct :

while (remaining && (frags->size != PAGE_SIZE ||
		     frags->page_offset)) {
	remaining -= frags->size;
	frags++;
}

My patch simply restore this code, keeping the accessors changes which were perfectly fine,
thank you.

Re: [PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy_receive()

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Oct 2019 20:19:59 -0700

> Apparently a refactoring patch brought a bug, that was caught
> by syzbot [1]
> 
> Original code was correct, do not try to be smarter than the
> compiler :/
> 
> [1]
 ...
> Fixes: d8e18a516f8f ("net: Use skb accessors in network core")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reported-by: syzbot <syzkaller@googlegroups.com>

Applied and queued up for -stable.