l2tp use-after-free fixes for 4.9 stable

Hi Greg,

Syzbot has been complaining about KASAN splats due to use-after-free issues in the l2tp code on 4.9 Android kernels (although I reproduced with latest 4.9 stable on my laptop):

https://syzkaller.appspot.com/bug?id=3c27eae7bdba97293b68e79c9700ac110f977eed

These have been fixed upstream, but for some reason didn't get picked up for stable. This series applies to 4.9.y and I'll send patches for 4.4.y separately as there are a few dependencies to deal with over there.

Thanks,

Will

--->8

Guillaume Nault (2):
l2tp: ensure sessions are freed after their PPPOL2TP socket
l2tp: fix race between l2tp_session_delete() and
l2tp_tunnel_closeall()

 net/l2tp/l2tp_core.c | 6 ++++++
 net/l2tp/l2tp_core.h | 1 +
 net/l2tp/l2tp_ppp.c  | 8 ++++----
 3 files changed, 11 insertions(+), 4 deletions(-)

2.26.0.rc2.310.g2932bb562d-goog

From: Guillaume Nault <[email protected]>

commit cdd10c9627496ad25c87ce6394e29752253c69d3 upstream.

If l2tp_tunnel_delete() or l2tp_tunnel_closeall() deletes a session right after pppol2tp_release() orphaned its socket, then the 'sock' variable of the pppol2tp_session_close() callback is NULL. Yet the session is still used by pppol2tp_release().

Therefore we need to take an extra reference in any case, to prevent l2tp_tunnel_delete() or l2tp_tunnel_closeall() from freeing the session.

Since the pppol2tp_session_close() callback is only set if the session is associated to a PPPOL2TP socket and that both l2tp_tunnel_delete() and l2tp_tunnel_closeall() hold the PPPOL2TP socket before calling pppol2tp_session_close(), we're sure that pppol2tp_session_close() and pppol2tp_session_destruct() are paired and called in the right order. So the reference taken by the former will be released by the later.

---
 net/l2tp/l2tp_ppp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 16b63e60396f..d919b3e6b548 100644
@@ -437,11 +437,11 @@ static void pppol2tp_session_close(struct l2tp_session *session)
 
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 
-	if (sock) {
+	if (sock)
 		inet_shutdown(sock, SEND_SHUTDOWN);
-		/* Don't let the session go away before our socket does */
-		l2tp_session_inc_refcount(session);
-	}
+
+	/* Don't let the session go away before our socket does */
+	l2tp_session_inc_refcount(session);
 }
 
 /* Really kill the session socket. (Called from sock_put() if

2.26.0.rc2.310.g2932bb562d-goog

Sorry, you will have to repost all of these l2tp patches with proper cover letters for a patch series.

In this particular case it's even more necessary, you posted two patch series. One with 2 patches and one with 8 patches. Do they depend upon eachother? If so, which one goes first. What is each patch series doing? How is it doing it? And why are you doing it that way?

Those are the questions answered by a properly written header posting.

Thank you.

Sorry, David. These two series are just backports for stable of patches that are already in mainline. Git added you to CC because of your SoB on the patches, but I don't think there's anything for you to do.

The 2-patch series is for 4.9-stable kernels.
The 8-patch series is for 4.4-stable kernels, and it's bigger because there are missing dependencies in 4.4-stable.

I did call this out in the cover letters but, of course, you weren't CC'd on those. Duh. Anyway, sorry again for the noise.

Will

From: Guillaume Nault <[email protected]>

commit b228a94066406b6c456321d69643b0d7ce11cfa6 upstream.

There are several ways to remove L2TP sessions:

* deleting a session explicitly using the netlink interface (with L2TP_CMD_SESSION_DELETE),
* deleting the session's parent tunnel (either by closing the tunnel's file descriptor or using the netlink interface),
* closing the PPPOL2TP file descriptor of a PPP pseudo-wire.

In some cases, when these methods are used concurrently on the same session, the session can be removed twice, leading to use-after-free bugs.

This patch adds a 'dead' flag, used by l2tp_session_delete() and l2tp_tunnel_closeall() to prevent them from stepping on each other's toes.

The session deletion path used when closing a PPPOL2TP file descriptor doesn't need to be adapted. It already has to ensure that a session remains valid for the lifetime of its PPPOL2TP file descriptor. So it takes an extra reference on the session in the ->session_close() callback (pppol2tp_session_close()), which is eventually dropped in the ->sk_destruct() callback of the PPPOL2TP socket (pppol2tp_session_destruct()).
Still, __l2tp_session_unhash() and l2tp_session_queue_purge() can be called twice and even concurrently for a given session, but thanks to proper locking and re-initialisation of list fields, this is not an issue.

---
 net/l2tp/l2tp_core.c | 6 ++++++
 net/l2tp/l2tp_core.h | 1 +
 2 files changed, 7 insertions(+)

index 394a1ddb0782..7c3da29fad8e 100644
@@ -1351,6 +1351,9 @@ void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 
 			hlist_del_init(&session->hlist);
 
+			if (test_and_set_bit(0, &session->dead))
+				goto again;
+
 			if (session->ref != NULL)
 				(*session->ref)(session);
 

@@ -1799,6 +1802,9 @@ EXPORT_SYMBOL_GPL(__l2tp_session_unhash);
  */
 int l2tp_session_delete(struct l2tp_session *session)
 {
+	if (test_and_set_bit(0, &session->dead))
+		return 0;
+
 	if (session->ref)
 		(*session->ref)(session);
 	__l2tp_session_unhash(session);

index 7cc49715606e..7c2037184b6c 100644
@@ -84,6 +84,7 @@ struct l2tp_session_cfg {
 struct l2tp_session {
 	int			magic;		/* should be
 						 * L2TP_SESSION_MAGIC */
+	long			dead;
 
 	struct l2tp_tunnel	*tunnel;	/* back pointer to tunnel
 						 * context */

2.26.0.rc2.310.g2932bb562d-goog

Thanks, both now queued up.

greg k-h