netdev - Re: [PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Oct 2019 04:12:01 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Matthew Wilcox <willy@...radead.org>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Soheil Hassas Yeganeh <soheil@...gle.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] tcp: fix slab-out-of-bounds in tcp_zerocopy_receive()

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.