netdev - Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

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:   Fri, 31 Jan 2020 13:58:14 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Ridge Kennedy <ridgek@...iedtelesis.co.nz>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

On Fri, Jan 17, 2020 at 10:26:09AM +1300, Ridge Kennedy wrote:
> On Thu, 16 Jan 2020, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > index f82ea12bac37..0cc86227c618 100644
> > > --- a/net/l2tp/l2tp_core.c
> > > +++ b/net/l2tp/l2tp_core.c
> > > @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
> > >  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
> > > 
> > >  		hlist_for_each_entry(session_walk, g_head, global_hlist)
> > > -			if (session_walk->session_id == session->session_id) {
> > > +			if (session_walk->session_id == session->session_id &&
> > > +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
> > > +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
> > >  				err = -EEXIST;
> > >  				goto err_tlock_pnlock;
> > >  			}
> > Well, I think we have a more fundamental problem here. By adding
> > L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> > sessions. That is, if we have an L2TPv3 session X running over UDP and
> > we receive an L2TP_IP packet targetted at session ID X, then
> > l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
> > 
> > I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> > look up the session in the context of its socket, like in the UDP case.
> > 
> > But for the moment, what about just not adding L2TP_UDP sessions to the
> > global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> > cross-talks.
> 
> I did consider not adding the L2TP_UDP sessions to the global list, but that
> change looked a little more involved as the netlink interface was also
> making use of the list to lookup sessions by ifname. At a minimum
> it looks like this would require rework of l2tp_session_get_by_ifname().
> 
Yes, you're right. Now that we all agree on this fix, can you please
repost your patch?

BTW, I wouldn't mind a small comment on top of the conditional to
explain that IP encap assumes globally unique session IDs while UDP
doesn't.