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

Message-ID: <20190124160107.GA6470@localhost.localdomain>
Date:   Thu, 24 Jan 2019 17:01:09 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Jacob Wen <jian.w.wen@...cle.com>
Cc:     netdev@...r.kernel.org, eric.dumazet@...il.com,
        g.nault@...halink.fr
Subject: Re: [PATCH net-next v2] net: l2tp: fix reading optional fields of
 L2TPv3

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.

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).
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.