Re: [PATCH] l2tp: Support several sockets with same IP/port quadruple

[James Chapman <jchapman@katalix.com>]

Hi Samuel,

On 06/05/2024 22:44, Samuel Thibault wrote:
> Hello,
>
> James Chapman, le ven. 03 mai 2024 12:36:14 +0100, a ecrit:
>
>>> +			/* We are receiving trafic for another tunnel, probably
>>> +			 * because we have several tunnels between the same
>>> +			 * IP/port quadruple, look it up.
>>> +			 */
>>> +			struct l2tp_tunnel *alt_tunnel;
>>> +
>>> +			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
>> This misses a check that alt_tunnel's protocol version matches the header.
>> Move the existing header version check to after this fragment?
> We need to check the version before getting the tunnel id, which we need
> to look up the struct l2tp_tunnel :)
I was referring to the following code fragment which is before your change:

 >    version = hdrflags & L2TP_HDR_VER_MASK;
 >    if (version != tunnel->version) {
 >        pr_debug_ratelimited("%s: recv protocol version mismatch: got 
%d expected %d\n",
 >                     tunnel->name, version, tunnel->version);
 >        goto invalid;
 >    }

The tunnel->version check should now be done after the tunnel pointer is 
possibly modified by your code.

Also, if the tunnel pointer from sk_user_data isn't trusted due to 
5-tuple aliasing, l2tp_udp_recv_core should compare with the local 
'version' variable, not tunnel->version, when parsing the L2TP IDs e.g.:

 >    if (version == L2TP_HDR_VER_2) {
 >        /* If length is present, skip it */

otherwise, L2TPv2 socket aliasing will still not work properly if one or 
more L2TPv3 sockets also alias L2TPv2 sockets, even if there is no 
L2TPv3 traffic.