netdev - Re: [PATCH net-next v2] net: l2tp: fix reading optional fields of L2TPv3

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] [day] [month] [year] [list]

Message-ID: <b5f80142-236f-2f54-d17d-3a87451a04e3@oracle.com>
Date:   Mon, 28 Jan 2019 14:15:14 +0800
From:   Jacob Wen <jian.w.wen@...cle.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next v2] net: l2tp: fix reading optional fields of
 L2TPv3



On 1/25/19 12:01 AM, Guillaume Nault wrote:
> On Thu, Jan 24, 2019 at 03:49:17PM +0800, Jacob Wen wrote:
>> Use pskb_may_pull() to make sure the optional fields are in skb linear
>> parts, so we can safely read them later.
>>
>> It's easy to reproduce the issue with a net driver that supports paged
>> skb data. Just create a L2TPv3 over IP tunnel and then generates some
>> network traffic.
>> Once reproduced, rx err in /sys/kernel/debug/l2tp/tunnels will increase.
>>
>> Signed-off-by: Jacob Wen <jian.w.wen@...cle.com>
>> ---
>> Changes in v2:
>> 1. Only fix L2TPv3 to make code simple.
>>     To fix both L2TPv3 and L2TPv2, we'd better refactor l2tp_recv_common.
>>     It's complicated to do so.
>>
> Yes, the L2TP data path definitely needs some care. But for a one-off
> patch like this, it'd probably make more sense to respect the current
> code structure instead of adding yet more special cases.
Agree. I didn't give l2tp_udp_recv_core enough attention.
> I mean, l2tp_recv_common() assumes that it can safely access the L2TP
> header: pskb_may_pull() is done in l2tp_udp_recv_core() (which probably
> should pull more bytes in case the length field is present BTW).
Yes. Two more bytes are required for L2TPv2.
> It's up to l2tp_ip (and l2tp_ip6) to respect this requirement, so that's
> where pskb_may_pull() should be done. Yes it'd be better to linearise
> data close to the place we access them, but that'd be long term
> refactoring. If we don't have the resources to do that, let's just, at
> least keep some consistency.
I will send a new patch.
Thanks.

-- 
Jacob