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] [day] [month] [year] [list]

Date:   Tue, 4 Feb 2020 12:29:11 +1300 (NZDT)
From:   Ridge Kennedy <ridgek@...iedtelesis.co.nz>
To:     Guillaume Nault <gnault@...hat.com>
cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] l2tp: Allow duplicate session creation with UDP


On Fri, 31 Jan 2020, Guillaume Nault wrote:

> 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.
>
Thanks all, I have reposted the patch with added comment.