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]

Date:   Tue, 2 Jan 2018 20:08:07 +0000
From:   James Chapman <jchapman@...alix.com>
To:     Guillaume Nault <g.nault@...halink.fr>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        davem@...emloft.net, netdev@...r.kernel.org, liuhangbin@...il.com
Subject: Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

On 02/01/18 17:50, Guillaume Nault wrote:
> On Fri, Dec 29, 2017 at 06:53:56PM +0000, James Chapman wrote:
>> On 28/12/17 19:45, Guillaume Nault wrote:
>>> Here we have an option that:
>>>     * creates invalid packets (AFAIK),
>>>     * is buggy and leaks memory on the network,
>>>     * doesn't seem to have any use case (even the manpage
>>>       says "This is hardly ever used").
>>>
>>> So I'm sorry, but I don't see the point in expanding this option to
>>> allow even stranger setups. If there's a use case, then fine.
>>> Otherwise, let's just acknowledge that the "peer_offset" option of
>>> iproute2 is a noop (and maybe remove it from the manpage).
>>>
>>> And the kernel "offset" option needs to be fixed. Actually, I wouldn't
>>> mind if it was converted to be a noop, or even rejected. L2TP already
>>> has its share of unused features that complicate the code and hamper
>>> evolution and bug fixing. As I said earlier, if it's buggy, unused and
>>> can't even produce valid packets, then why bothering with it?
>>>
>>> But that's just my point of view. James, do you have an opinion on
>>> this?
>> I agree, Guillaume.
>>
>> The L2TPv3 protocol RFC dropped the configurable offset of L2TPv2 - instead,
>> the Layer-2-Specific-Sublayer is supposed to handle any transport-specific
>> data alignment requirements.
>>
> Yes, and AFAIK, no RFC has ever defined an L2TPv3 sublayer using offsets.
>
>> I think a configurable offset has found its way
>> into iproute2 l2tp commands by mistake, perhaps because the netlink API
>> defines an attribute for it, but which was only intended for use with
>> L2TPv2.
>>
> Makes sense, however L2TP_ATTR_OFFSET seems to be a noop for L2TPv2 in
> the current implementation.
>
>> For L2TPv2, we only configure the offset for transmitted packets. In
>> received packets, the offset (if present) is obtained from the L2TPv2 header
>> in each received packet. There is no need to add a peer-offset netlink
>> attribute to set the offset expected in received packets.
>>
> Agreed for Rx side. I also agree on the theory for Tx, but in the current
> implementation, l2tp_build_l2tpv2_header() doesn't take the session's
> "offset" field into account. So, unless I've missed something,
> L2TP_ATTR_OFFSET is already a noop for L2TPv2.

You're right. My bad.

> Not sure if it's worth handling this feature of L2TPv2. The Linux
> implementation has been there for so long, and nobody ever complained
> that there was no way to define an offset on Tx.

I agree.