netdev - Re: [PATCH] net: make sure skb

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:   Wed, 24 Jan 2018 16:37:56 +0100
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     Roman Kapl <code@...pl.cz>, Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jiri Benc <jbenc@...hat.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Eric Dumazet <edumazet@...gle.com>,
        David Ahern <dsahern@...il.com>, Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH] net: make sure skb_dst is valid before using

Le 24/01/2018 à 10:49, Roman Kapl a écrit :
> On 01/24/2018 10:16 AM, Xin Long wrote:
>> On Wed, Jan 24, 2018 at 6:42 AM, Roman Kapl <code@...pl.cz> wrote:
>>> Tunnel devices often use skb_dst(skb)->ops, but ops are not implemented
>>> for metadata tunnel destinations. Use skb_valid_dst to check if skb_dst
>>> is a real (non-metadata) destination.
>>>
>>> Such packets can easily be crafted using tc tunnel_key actions or BPF
>>> and will crash the tunnels, as observed at least with sit, geneve and
>>> vxlan. Some of these tunnels are also intended to be used with metadata
>>> destinations, so this represents a loss of functionality.
>>>
>>> There might be more places with similar patterns, but sometimes it is
>>> hard to tell if metadata dst packets can reach them, so this patch is
>>> not exhaustive.
>> This patch is trying to fix a lot of places, and there may be more.
>> But all because of the lack of .update_pmtu or .neigh_lookup.
>> (dst_link_failure is safe, btw)
>>
>> Not sure if it will be better to add .update_pmtu, .neigh_lookup and
>> even .redirect for md_dst_ops, but just leave them empty ?
>> like fake_dst_ops in br_nf.
> That's what I was suggesting in the original mail. However, it might have slight
> impact on performance. Also it was not done when metadata destinations were
> originally added, was there any good reason?
>> Let's see other's opinions.
> Exactly.
I would prefer a patch that test if the handler is available. It would prevent
to have this bug again in the future. But I don't have a strong opinion about this.

But it would be nice to send a patch quickly, before the 4.15 release, to fix
this crash.


Thank you,
Nicolas