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

[James Chapman]

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.