[PATCH 0/2] [backports] l2tp use-after-free fixes for 4.9 stable

[PATCH 0/2] [backports] l2tp use-after-free fixes for 4.9 stable

[Will Deacon]

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

[PATCH 1/2] l2tp: ensure sessions are freed after their PPPOL2TP socket

[Will Deacon]

From: Guillaume Nault <g.nault@alphalink.fr>

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.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Will Deacon <will@kernel.org>
---
 net/l2tp/l2tp_ppp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 16b63e60396f..d919b3e6b548 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -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

Re: [PATCH 1/2] l2tp: ensure sessions are freed after their PPPOL2TP socket

[David Miller]

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.

Re: [PATCH 1/2] l2tp: ensure sessions are freed after their PPPOL2TP socket

[Will Deacon]

On Thu, Apr 02, 2020 at 12:59:30PM -0700, David Miller wrote:
> 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.

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

[PATCH 2/2] l2tp: fix race between l2tp_session_delete() and l2tp_tunnel_closeall()

[Will Deacon]

From: Guillaume Nault <g.nault@alphalink.fr>

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.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Will Deacon <will@kernel.org>
---
 net/l2tp/l2tp_core.c | 6 ++++++
 net/l2tp/l2tp_core.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 394a1ddb0782..7c3da29fad8e 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -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);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 7cc49715606e..7c2037184b6c 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -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

Re: [PATCH 0/2] [backports] l2tp use-after-free fixes for 4.9 stable

[Greg KH]

On Thu, Apr 02, 2020 at 06:31:56PM +0100, Will Deacon wrote:
> 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, both now queued up.

greg k-h