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

[Paolo Abeni]

On Mon, 2024-07-08 at 14:57 +0100, James Chapman wrote:
> On 08/07/2024 12:59, Hillf Danton wrote:
> > On Mon, 8 Jul 2024 11:06:25 +0100 James Chapman <jchapman@xxxxxxxxxxx>
> > > On 05/07/2024 11:32, Hillf Danton wrote:
> > > > On Thu,  4 Jul 2024 16:25:08 +0100 James Chapman <jchapman@xxxxxxxxxxx>
> > > > > --- a/net/l2tp/l2tp_core.c
> > > > > +++ b/net/l2tp/l2tp_core.c
> > > > > @@ -1290,17 +1290,20 @@ static void l2tp_session_unhash(struct l2tp_session *session)
> > > > >    static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
> > > > >    {
> > > > >    	struct l2tp_session *session;
> > > > > -	struct list_head *pos;
> > > > > -	struct list_head *tmp;
> > > > >    
> > > > >    	spin_lock_bh(&tunnel->list_lock);
> > > > >    	tunnel->acpt_newsess = false;
> > > > > -	list_for_each_safe(pos, tmp, &tunnel->session_list) {
> > > > > -		session = list_entry(pos, struct l2tp_session, list);
> > > > > +	for (;;) {
> > > > > +		session = list_first_entry_or_null(&tunnel->session_list,
> > > > > +						   struct l2tp_session, list);
> > > > > +		if (!session)
> > > > > +			break;
> > > > > +		l2tp_session_inc_refcount(session);
> > > > >    		list_del_init(&session->list);
> > > > >    		spin_unlock_bh(&tunnel->list_lock);
> > > > >    		l2tp_session_delete(session);
> > > > >    		spin_lock_bh(&tunnel->list_lock);
> > > > > +		l2tp_session_dec_refcount(session);
> > > > 
> > > > Bumping refcount up makes it safe for the current cpu to go thru race
> > > > after releasing lock, and if it wins the race, dropping refcount makes
> > > > the peer head on uaf.
> > > 
> > > Thanks for reviewing this. Can you elaborate on what you mean by "makes
> > > the peer head on uaf", please?
> > > 
> > Given race, there are winner and loser. If the current cpu wins the race,
> > the loser hits uaf once winner drops refcount.
> 
> I think the session's dead flag would protect against threads racing in 
> l2tp_session_delete to delete the same session.
> Any thread with a pointer to a session should hold a reference on it to 
> prevent the session going away while it is accessed. Am I missing a 
> codepath where that's not the case?

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.

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.

Thanks,

Paolo