Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On  Thu, Jul 04, 2024 at 19:23:03 +0800, Hillf Danton wrote:
> On Wed, 3 Jul 2024 13:39:25 +0100 Tom Parkin <tparkin@katalix.com>
> > 
> > The specific UAF that syzbot hits is due to the list node that the
> > list_for_each_safe temporary variable points to being modified while
> > the list_for_each_safe walk is in process.
> > 
> > This is possible due to l2tp_tunnel_closeall dropping the spin lock
> > that protects the list mid way through the list_for_each_safe loop.
> > This opens the door for another thread to call list_del_init on the
> > node that list_for_each_safe is planning to process next, causing
> > l2tp_tunnel_closeall to repeatedly iterate on that node forever.
> > 
> Yeah the next node could race with other thread because of the door.

Exactly, yes.

> > In the context of l2tp_ppp, this eventually leads to UAF because the
> > session structure itself is freed when the pppol2tp socket is
> > destroyed and the pppol2tp sk_destruct handler unrefs the session
> > structure to zero.
> > 
> > So to avoid the UAF, the list can safely be processed using a loop
> > which accesses the first entry in the tunnel session list under
> > spin lock protection, removing that entry, then dropping the lock
> > to call l2tp_session_delete.
> 
> Race exists after your patch.
> 
> 	cpu1				cpu2
> 	---				---
> 					pppol2tp_release()
> 
> 	spin_lock_bh(&tunnel->list_lock);
> 	while (!list_empty(&tunnel->session_list)) {
> 		session = list_first_entry(&tunnel->session_list,
> 					struct l2tp_session, list);
>  		list_del_init(&session->list);
>  		spin_unlock_bh(&tunnel->list_lock);
> 
>  					l2tp_session_delete(session);
> 
>  		l2tp_session_delete(session);
>  		spin_lock_bh(&tunnel->list_lock);
>  	}
>  	spin_unlock_bh(&tunnel->list_lock);

I take your point.  Calling l2tp_session_delete() on the same session
twice isn't a problem per-se, but if cpu2 manages to destruct the
socket and unref the session to zero before cpu1 progresses then we
have the same sort of problem.

To be honest, cleanup generally could use some TLC (the dancing around
the list lock in _closeall is not ideal), but a minimal patch to
address the UAF makes sense in the short term IMO -- so to that end
holding a session reference around l2tp_session_delete in _closeall is
probably the least invasive thing to do.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]