netdev - Re: [PATCH net-next 2/2] l2tp: add peer

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: <95029884-ced2-253a-efb4-81cda15cbfe7@katalix.com>
Date:   Tue, 2 Jan 2018 20:18:27 +0000
From:   James Chapman <jchapman@...alix.com>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Guillaume Nault <g.nault@...halink.fr>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

On 02/01/18 19:28, Lorenzo Bianconi wrote:
>>>> Lorenzo, is this being added to fix interoperability with another L2TPv3
>>>> implementation? If so, can you share more details?
>>>>
>>> Hi James,
>>>
>>> I introduced peer_offset parameter to fix a specific setup where
>>> tunnel endpoints
>>> running L2TPv3 would use different values for tx offset (since in
>>> iproute2 there is no
>>> restriction on it), not to fix a given an interoperability issue.
>>>
>> Yes, but was it just to test iproute2's peer_offset option? Or is there
>> a plan to use it for real?
>>
>>> I introduced this feature since:
>>>   - offset has been added for long time to L2TPv3 implementation
>>>     (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
>>>     commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
>>> preserve UABI
>>>   - have the same degree of freedom for offset parameter we have in
>>> L2TPv2 and fix the issue
>>>     described above
>>>
>> AFAIU, the current L2TPv2 implementation never sets the offset field
>> and nobody ever realised.
>>
> Perhaps I am little bit polarized on UABI issue, but I was rethinking
> about it and maybe removing offset parameter would lead to an
> interoperability issue for device running L2TPv3 since offset
> parameter is there and it is not a nope.
> Please consider this setup:
> - 2 endpoint running L2TPv3, the first running net-next and the second
> running 4.14
> - both endpoint are configured using iproute2 in this way:
>
>    - ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>    - ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>    - ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
> peer_session_id <s1> offset 8
>    - ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
> peer_session_id <s0> offset 8
>
> Can we assume offset is never used for L2TPv3?
If offset is ever set non-zero, packets transmitted on the wire are not 
compliant with the L2TPv3 RFC. So if someone is using a non-zero offset 
in their config, it might be a good thing that it stops working with a 
kernel update.

> Regards,
> Lorenzo
>
>>> Now what we can do I guess is:
>>> - as suggested by Guillaume drop completely the offset support without removing
>>>    netlink attribute in order to not break UABI
>>> - fix offset support initializing properly padding bits
>>>
>> I'd go for the first one. I just wonder if that looks acceptable to
>> David an James.