netdev - Re: net: GPF in __netlink_ns

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]

Message-ID: <878u3m7i5q.fsf@x220.int.ebiederm.org>
Date:	Mon, 18 Jan 2016 14:21:37 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	Richard Weinberger <richard.weinberger@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Thomas Graf <tgraf@...g.ch>,
	Daniel Borkmann <daniel@...earbox.net>,
	Ken-ichirou MATSUZAWA <chamaken@...il.com>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	Florian Westphal <fw@...len.de>,
	netdev <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: net: GPF in __netlink_ns_capable

Herbert Xu <herbert@...dor.apana.org.au> writes:

> On Sat, Jan 16, 2016 at 01:08:33AM +0100, Richard Weinberger wrote:
>> On Fri, Jan 15, 2016 at 11:31 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
>> > Call Trace:
>> >  [<     inline     >] netlink_ns_capable net/netlink/af_netlink.c:1417
>> >  [<ffffffff8529c0a5>] netlink_capable+0x25/0x30 net/netlink/af_netlink.c:1432
>> 
>> Hmm, we're crashing because NETLINK_CB(skb).sk is NULL.
>> netlink_dump() creates a new skb without a netlink control block,
>> but infiniband's dump functions use netlink_capable() which needs a valid
>> NETLINK_CB(skb).sk.
>> 
>> What about something like that?
>
> No you can't do it here as netlink_unicast also calls this and for
> that case you'd be overwriting the original sending user-space
> socket with the kernel socket.
>
> I'm adding Eric Bierderman as he wrote some of the code in question.

*Scratches my head*

I think I am just going to recommend removing that bit of code from
the infiniband stack.

There are several things very wrong here.

First netlinnk_capable and it's ilk are for the very specific purpose of
handling backwards compatibility as a truly clean solution of checking
at open or connect time would break existing applications.
ib_nl_handle_resolv_resp is new code.  So it can almost certainly do
something cleaner.

netlink_capable is very much not for filtering netlink dumps, but for
filtering the queries themselves.  So it appears the capability check is
very much in the wrong place.

All of this is newish code and apparently never even tested as this code
should have failed this way for everyone.  So since the code does not
work not apparently has never worked, it is probably easiest just to
remove the problematic code (AKA revert), and start fresh and not
something that requires backwards compatibility hacks from day one.

By new I mean code that came in through the commit below.

commit 2ca546b92a024d07adedd15b4c262b1c2c0786ec
Author: Kaike Wan <kaike.wan@...el.com>
Date:   Fri Aug 14 08:52:09 2015 -0400

    IB/sa: Route SA pathrecord query through netlink
    
    This patch routes a SA pathrecord query to netlink first and processes the
    response appropriately. If a failure is returned, the request will be sent
    through IB. The decision whether to route the request to netlink first is
    determined by the presence of a listener for the local service netlink
    multicast group. If the user-space local service netlink multicast group
    listener is not present, the request will be sent through IB, just like
    what is currently being done.
    
    Signed-off-by: Kaike Wan <kaike.wan@...el.com>
    Signed-off-by: John Fleck <john.fleck@...el.com>
    Signed-off-by: Ira Weiny <ira.weiny@...el.com>
    Signed-off-by: Doug Ledford <dledford@...hat.com>

Eric