netdev - Re: [PATCH net-next v2] l2tp: fix possible UAF when cleaning up tunnels

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: <9d86ddab-5177-b534-11be-6609065a2ab9@katalix.com>
Date: Tue, 9 Jul 2024 10:29:16 +0100
From: James Chapman <jchapman@...alix.com>
To: Paolo Abeni <pabeni@...hat.com>, Hillf Danton <hdanton@...a.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 syzkaller-bugs@...glegroups.com, tparkin@...alix.com,
 syzbot+b471b7c936301a59745b@...kaller.appspotmail.com,
 syzbot+c041b4ce3a6dfd1e63e2@...kaller.appspotmail.com
Subject: Re: [PATCH net-next v2] l2tp: fix possible UAF when cleaning up
 tunnels

On 09/07/2024 10:03, Paolo Abeni wrote:
[snip]
> AFAICS this patch is safe, as the session refcount can't be 0 at
> l2tp_session_inc_refcount() time and will drop to 0 after
> l2tp_session_dec_refcount() only if no other entity/thread is owning
> any reference to the session.
> 
> @James: the patch has a formal issue, you should avoid any empty line
> in the tag area, specifically between the 'Fixes' and SoB tags.
> 
> I'll exceptionally fix this while applying the patch, but please run
> checkpatch before your next submission.

Thanks Paolo. Will do. I'll be more careful next time.

> Also somewhat related, I think there is still a race condition in
> l2tp_tunnel_get_session():
> 
> 	rcu_read_lock_bh();
>          hlist_for_each_entry_rcu(session, session_list, hlist)
>                  if (session->session_id == session_id) {
>                          l2tp_session_inc_refcount(session);
> 
> I think that at l2tp_session_inc_refcount(), the session refcount could
> be 0 due to a concurrent tunnel cleanup. l2tp_session_inc_refcount()
> should likely be refcount_inc_not_zero() and the caller should check
> the return value.
> 
> In any case the latter is a separate issue.

I'm currently working on another series which will address this along 
with more l2tp cleanup improvements.