netdev - Re: ip_rcv_finish() NULL pointer kernel panic

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, 26 Jan 2017 13:04:28 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     fw@...len.de
Cc:     eric.dumazet@...il.com, lkml@...ene.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org
Subject: Re: ip_rcv_finish() NULL pointer kernel panic

From: Florian Westphal <fw@...len.de>
Date: Thu, 26 Jan 2017 17:24:33 +0100

> Eric Dumazet <eric.dumazet@...il.com> wrote:
>> > Though possibly with different things not setting the "input" function 
>> > pointer in the "struct dst_entry".
>> > 
>> > include/net/dst.h:
>> >   496 static inline int dst_input(struct sk_buff *skb) {
>> >   498         return skb_dst(skb)->input(skb);
>> >   499 }
>> > 
>> > Is there any reason not to check to see if this pointer is NULL before 
>> > blindly calling it ?
>> 
>> Sure. It can not be NULL at this point.
>> 
>> Just look at the code in ip_rcv_finish() :
>> 
>> It first make sure to get a valid dst.
>> 
>> Something is broken, probably in bridge netfilter code.
>> 
>> I suspect the dst here points to &br->fake_rtable, and this is not
>> expected.
>> 
>> br_drop_fake_rtable() should have been called somewhere ...
> 
> I think it makes sense to set dst->incoming
> to a stub in br_netfilter_rtable_init() to just kfree_skb()+
> WARN_ON_ONCE(), no need to add code to ip stack or crash kernel
> due to brnf bug.

That would certainly make recovery from such bugs must better.

But I have to say that this netfilter bridging fake dst has caused
several dozen bugs over the years, it is fundamentally a serious
problem in and of itself.  It provides DST facilities by hand, in a
static object, without using any of the usual methods for creating and
facilitating dst objects.

Therefore every time someone makes an adjustment to common dst code,
this turd (and yes, it _is_ a turd) breaks.  Every single time.

So in the long term, instead of polishing this turd, let's get rid of
it.