Re: HARDENED_USERCOPY will BUG on multiple slub objects coalesced into an sk_buff fragment

[Matthew Wilcox <willy@infradead.org>]

On Fri, Jun 01, 2018 at 01:49:38PM -0700, Kees Cook wrote:
> On Fri, Jun 1, 2018 at 12:02 PM, Laura Abbott <labbott@redhat.com> wrote:
> > (cc-ing some interested people)
> >
> >
> >
> > On 05/31/2018 05:03 PM, Anton Eidelman wrote:
> >> Here's a rare issue I reproduce on 4.12.10 (centos config): full log
> >> sample below.
> 
> Thanks for digging into this! Do you have any specific reproducer for
> this? If so, I'd love to try a bisection, as I'm surprised this has
> only now surfaced: hardened usercopy was introduced in 4.8 ...
> 
> >> An innocent process (dhcpclient) is about to receive a datagram, but
> >> during skb_copy_datagram_iter() usercopy triggers a BUG in:
> >> usercopy.c:check_heap_object() -> slub.c:__check_heap_object(), because
> >> the sk_buff fragment being copied crosses the 64-byte slub object boundary.
> >>
> >> Example __check_heap_object() context:
> >>    n=128    << usually 128, sometimes 192.
> >>    object_size=64
> >>    s->size=64
> >>    page_address(page)=0xffff880233f7c000
> >>    ptr=0xffff880233f7c540
> >>
> >> My take on the root cause:
> >>    When adding data to an skb, new data is appended to the current
> >> fragment if the new chunk immediately follows the last one: by simply
> >> increasing the frag->size, skb_frag_size_add().
> >>    See include/linux/skbuff.h:skb_can_coalesce() callers.
> 
> Oooh, sneaky:
>                 return page == skb_frag_page(frag) &&
>                        off == frag->page_offset + skb_frag_size(frag);
> 
> Originally I was thinking that slab red-zoning would get triggered
> too, but I see the above is checking to see if these are precisely
> neighboring allocations, I think.
> 
> But then ... how does freeing actually work? I'm really not sure how
> this seeming layering violation could be safe in other areas?

I'm confused ... I thought skb frags came from the page_frag allocator,
not the slab allocator.  But then why would the slab hardening trigger?

> > The analysis makes sense. Kees, any thoughts about what
> > we might do? It seems unlikely we can fix the networking
> > code so do we need some kind of override in usercopy?
> 
> If this really is safe against kfree(), then I'd like to find the
> logic that makes it safe and either teach skb_can_coalesce() different
> rules (i.e. do not cross slab objects) or teach __check_heap_object()
> about skb... which seems worse. Wheee.
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security
>