netdev - Re: [PATCH] tcp: detect use sendpage for slab-based objects

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 22 Feb 2019 08:39:22 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Vasily Averin <vvs@...tuozzo.com>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] tcp: detect use sendpage for slab-based objects

On Fri, Feb 22, 2019 at 6:02 AM Vasily Averin <vvs@...tuozzo.com> wrote:
>
> On 2/21/19 7:00 PM, Eric Dumazet wrote:
> > On Thu, Feb 21, 2019 at 7:30 AM Vasily Averin <vvs@...tuozzo.com> wrote:
> >> index 2079145a3b7c..cf9572f4fc0f 100644
> >> --- a/net/ipv4/tcp.c
> >> +++ b/net/ipv4/tcp.c
> >> @@ -996,6 +996,7 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
> >>                         goto wait_for_memory;
> >>
> >>                 if (can_coalesce) {
> >> +                       WARN_ON_ONCE(PageSlab(page));
> >
> > Please use VM_WARN_ON_ONCE() to make this a nop for CONFIG_VM_DEBUG=n
> >
> > Also the whole tcp_sendpage() should be protected, not only the coalescing part.
> >
> > (The get_page()  done few lines later should not be attempted either)
> >
> >>                         skb_frag_size_add(&skb_shinfo(skb)->frags[i - 1], copy);
> >>                 } else {
> >>                         get_page(page);
> >> --
> >
> > It seems the bug has nothing to do with TCP, and belongs to the caller.
> >
> > Otherwise you need to add the check to all existing .sendpage() /
> > .sendpage_locked() handler out there.
>
> Eric, could you please elaborate once again why tcp_sendpage() should not handle slab objects?

Simply because SLAB has its own way to manage objects from a page, and
does not care
about the underlying page having its refcount elevated.

ptr = kmalloc(xx)
...  < here you can attempt cheating and add one to the underlying page>
kfree(ptr); // SLAB does not care of page count, it will effectively
put ptr in the free list.

ptr2 = kmalloc(xx); //

ptr2 can be the same than ptr (object was kfreed() earlier)

This means that some other stuff will happily reuse the piece of
memory that you wanted to use for zero-copy.

This is a serious bug IMO, since this would allow for data corruption.



>
> There is known restriction: sendpage should not be called for pages with counter=0,
> because internal put_page() releases the page. All sendpage callers I know have such check.
>
> However why they should add one check for PageSlab?
>
> Let me explain the problem once again:
> I do not see any bugs neither in tcp nor in any sendpage callers,
> there is false alert on receiving side that crashes correctly worked host.

This is not a false alert, but a very fundamental issue.

We can not mix kmalloc() and page fragments, this simply is buggy.

>
> There is network block device with XFS,
> XFS submit IO request with slab objects,
> block device driver checks that page count is positive and decides to use sendpage.
> sendpage calls tcp_sendpage() that can merge 2 neighbour slab objects into one tcp fragment.
>
> If data is transferred outside -- nothing bad happen, network device successfully send data outside.
> However if data is received locally tcp_recvmsg detects strange vector with "merged" slab objects.
> It is not real problem, data can be accessed correctly, however this check calls BUG_ON and crashes the host.
>
> By this way recently added hardening check forces all .sendpage callers modify code that worked correctly for ages.
>
> It looks abnormal to me, but I do not understand how to fix this problem correctly.
>
> I do not like an idea to keep current state -- it can trigger crash of correctly worked hosts in some rare corner cases.
> I do not like an idea to fix all callers -- why they need modify correctly worked code to protect from false positive?
> I do not like an idea to modify tcp -- to block merge of fragments with slab objects like I proposed earlier.
> We can trigger warning in tcp code -- to inform .sendpage callers that they are under fire,
> however I agree with yours "bug has nothing to do with TCP" and do not understand why we need to modify tcp_sendpage().
>
> May be it's better to replace BUG_ON to WARN_ON in hardening check?
> Could you probably advise some other solution?
>
> Thank you,
>         Vasily Averin