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

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: <20240506214424.4wddiwjdpdl2gf4w@begin>
Date: Mon, 6 May 2024 23:44:24 +0200
From: Samuel Thibault <samuel.thibault@...-lyon.org>
To: James Chapman <jchapman@...alix.com>
Cc: Tom Parkin <tparkin@...alix.com>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH] l2tp: Support several sockets with same IP/port quadruple

Hello,

James Chapman, le ven. 03 mai 2024 12:36:14 +0100, a ecrit:
> > @@ -845,6 +846,20 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
> >   		/* Extract tunnel and session ID */
> >   		tunnel_id = ntohs(*(__be16 *)ptr);
> >   		ptr += 2;
> > +
> > +		if (tunnel_id != tunnel->tunnel_id && tunnel->l2tp_net) {
> Can tunnel->l2tp_net be NULL?

l2tp_tunnel_sock_create's comment says

 * Since we don't want these sockets to keep a namespace alive by
 * themselves, we drop the socket's namespace refcount after creation.
 * These sockets are freed when the namespace exits using the pernet
 * exit hook.

and l2tp_tunnel_create does not set l2tp_net, only l2tp_tunnel_register
does, so I assumed it might be NULL and preferred to stay on
the safe side. But it's l2tp_tunnel_register which adds it to
pn->l2tp_tunnel_idr, so AIUI it indeed cannot be NULL since we got it
from pn->l2tp_tunnel_idr, we can probably drop the test indeed.

> > +			/* 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'll add another version check.

Samuel

> > +			if (!alt_tunnel)
> > +				goto pass;
> > +			tunnel = alt_tunnel;
> > +		}
> > +
> >   		session_id = ntohs(*(__be16 *)ptr);
> >   		ptr += 2;
> >   	} else {