| From b228a94066406b6c456321d69643b0d7ce11cfa6 Mon Sep 17 00:00:00 2001 |
| From: Guillaume Nault <g.nault@alphalink.fr> |
| Date: Fri, 22 Sep 2017 15:39:24 +0200 |
| Subject: l2tp: fix race between l2tp_session_delete() and l2tp_tunnel_closeall() |
| |
| 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> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| net/l2tp/l2tp_core.c | 6 ++++++ |
| net/l2tp/l2tp_core.h | 1 + |
| 2 files changed, 7 insertions(+) |
| |
| --- a/net/l2tp/l2tp_core.c |
| +++ b/net/l2tp/l2tp_core.c |
| @@ -1351,6 +1351,9 @@ again: |
| |
| 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); |
| --- 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 */ |