[PATCH net 0/3] l2tp: remove unsafe calls to l2tp_tunnel_find_nth()

[PATCH net 0/3] l2tp: remove unsafe calls to l2tp_tunnel_find_nth()

[Guillaume Nault]

Using l2tp_tunnel_find_nth() is racy, because the returned tunnel can
go away as soon as this function returns. This series introduce
l2tp_tunnel_get_nth() as a safe replacement to fixes these races.

With this series, all unsafe tunnel/session lookups are finally gone.

Guillaume Nault (3):
  l2tp: hold reference on tunnels in netlink dumps
  l2tp: hold reference on tunnels printed in pppol2tp proc file
  l2tp: hold reference on tunnels printed in l2tp/tunnels debugfs file

 net/l2tp/l2tp_core.c    | 40 ++++++++++++++++++++--------------------
 net/l2tp/l2tp_core.h    |  3 ++-
 net/l2tp/l2tp_debugfs.c | 15 +++++++++++++--
 net/l2tp/l2tp_netlink.c | 11 ++++++++---
 net/l2tp/l2tp_ppp.c     | 24 +++++++++++++++++-------
 5 files changed, 60 insertions(+), 33 deletions(-)

-- 
2.17.0

[PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps

[Guillaume Nault]

l2tp_tunnel_find_nth() is unsafe: no reference is held on the returned
tunnel, therefore it can be freed whenever the caller uses it.
This patch defines l2tp_tunnel_get_nth() which works similarly, but
also takes a reference on the returned tunnel. The caller then has to
drop it after it stops using the tunnel.

Convert netlink dumps to make them safe against concurrent tunnel
deletion.

Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
 net/l2tp/l2tp_core.c    | 20 ++++++++++++++++++++
 net/l2tp/l2tp_core.h    |  2 ++
 net/l2tp/l2tp_netlink.c | 11 ++++++++---
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 0fbd3ee26165..c8c4183f0f37 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -183,6 +183,26 @@ struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)
 }
 EXPORT_SYMBOL_GPL(l2tp_tunnel_get);
 
+struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth)
+{
+	const struct l2tp_net *pn = l2tp_pernet(net);
+	struct l2tp_tunnel *tunnel;
+	int count = 0;
+
+	rcu_read_lock_bh();
+	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
+		if (++count > nth) {
+			l2tp_tunnel_inc_refcount(tunnel);
+			rcu_read_unlock_bh();
+			return tunnel;
+		}
+	}
+	rcu_read_unlock_bh();
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(l2tp_tunnel_get_nth);
+
 /* Lookup a session. A new reference is held on the returned session. */
 struct l2tp_session *l2tp_session_get(const struct net *net,
 				      struct l2tp_tunnel *tunnel,
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index ba33cbec71eb..e4896413b2b6 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -212,6 +212,8 @@ static inline void *l2tp_session_priv(struct l2tp_session *session)
 }
 
 struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
+struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth);
+
 void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
 
 struct l2tp_session *l2tp_session_get(const struct net *net,
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index b05dbd9ffcb2..6616c9fd292f 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -487,14 +487,17 @@ static int l2tp_nl_cmd_tunnel_dump(struct sk_buff *skb, struct netlink_callback
 	struct net *net = sock_net(skb->sk);
 
 	for (;;) {
-		tunnel = l2tp_tunnel_find_nth(net, ti);
+		tunnel = l2tp_tunnel_get_nth(net, ti);
 		if (tunnel == NULL)
 			goto out;
 
 		if (l2tp_nl_tunnel_send(skb, NETLINK_CB(cb->skb).portid,
 					cb->nlh->nlmsg_seq, NLM_F_MULTI,
-					tunnel, L2TP_CMD_TUNNEL_GET) < 0)
+					tunnel, L2TP_CMD_TUNNEL_GET) < 0) {
+			l2tp_tunnel_dec_refcount(tunnel);
 			goto out;
+		}
+		l2tp_tunnel_dec_refcount(tunnel);
 
 		ti++;
 	}
@@ -848,7 +851,7 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback
 
 	for (;;) {
 		if (tunnel == NULL) {
-			tunnel = l2tp_tunnel_find_nth(net, ti);
+			tunnel = l2tp_tunnel_get_nth(net, ti);
 			if (tunnel == NULL)
 				goto out;
 		}
@@ -856,6 +859,7 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback
 		session = l2tp_session_get_nth(tunnel, si);
 		if (session == NULL) {
 			ti++;
+			l2tp_tunnel_dec_refcount(tunnel);
 			tunnel = NULL;
 			si = 0;
 			continue;
@@ -865,6 +869,7 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback
 					 cb->nlh->nlmsg_seq, NLM_F_MULTI,
 					 session, L2TP_CMD_SESSION_GET) < 0) {
 			l2tp_session_dec_refcount(session);
+			l2tp_tunnel_dec_refcount(tunnel);
 			break;
 		}
 		l2tp_session_dec_refcount(session);
-- 
2.17.0

[PATCH net 2/3] l2tp: hold reference on tunnels printed in pppol2tp proc file

[Guillaume Nault]

Use l2tp_tunnel_get_nth() instead of l2tp_tunnel_find_nth(), to be safe
against concurrent tunnel deletion.

Unlike sessions, we can't drop the reference held on tunnels in
pppol2tp_seq_show(). Tunnels are reused across several calls to
pppol2tp_seq_start() when iterating over sessions. These iterations
need the tunnel for accessing the next session. Therefore the only safe
moment for dropping the reference is just before searching for the next
tunnel.

Normally, the last invocation of pppol2tp_next_tunnel() doesn't find
any new tunnel, so it drops the last tunnel without taking any new
reference. However, in case of error, pppol2tp_seq_stop() is called
directly, so we have to drop the reference there.

Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
 net/l2tp/l2tp_ppp.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 896bbca9bdaa..7d0c963680e6 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1551,16 +1551,19 @@ struct pppol2tp_seq_data {
 
 static void pppol2tp_next_tunnel(struct net *net, struct pppol2tp_seq_data *pd)
 {
+	/* Drop reference taken during previous invocation */
+	if (pd->tunnel)
+		l2tp_tunnel_dec_refcount(pd->tunnel);
+
 	for (;;) {
-		pd->tunnel = l2tp_tunnel_find_nth(net, pd->tunnel_idx);
+		pd->tunnel = l2tp_tunnel_get_nth(net, pd->tunnel_idx);
 		pd->tunnel_idx++;
 
-		if (pd->tunnel == NULL)
-			break;
+		/* Only accept L2TPv2 tunnels */
+		if (!pd->tunnel || pd->tunnel->version == 2)
+			return;
 
-		/* Ignore L2TPv3 tunnels */
-		if (pd->tunnel->version < 3)
-			break;
+		l2tp_tunnel_dec_refcount(pd->tunnel);
 	}
 }
 
@@ -1609,7 +1612,14 @@ static void *pppol2tp_seq_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void pppol2tp_seq_stop(struct seq_file *p, void *v)
 {
-	/* nothing to do */
+	struct pppol2tp_seq_data *pd = v;
+
+	if (!pd || pd == SEQ_START_TOKEN)
+		return;
+
+	/* Drop reference taken by last invocation of pppol2tp_next_tunnel() */
+	if (pd->tunnel)
+		l2tp_tunnel_dec_refcount(pd->tunnel);
 }
 
 static void pppol2tp_seq_tunnel_show(struct seq_file *m, void *v)
-- 
2.17.0

[PATCH net 3/3] l2tp: hold reference on tunnels printed in l2tp/tunnels debugfs file

[Guillaume Nault]

Use l2tp_tunnel_get_nth() instead of l2tp_tunnel_find_nth(), to be safe
against concurrent tunnel deletion.

Use the same mechanism as in l2tp_ppp.c for dropping the reference
taken by l2tp_tunnel_get_nth(). That is, drop the reference just
before looking up the next tunnel. In case of error, drop the last
accessed tunnel in l2tp_dfs_seq_stop().

That was the last use of l2tp_tunnel_find_nth().

Fixes: 0ad6614048cf ("l2tp: Add debugfs files for dumping l2tp debug info")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
 net/l2tp/l2tp_core.c    | 20 --------------------
 net/l2tp/l2tp_core.h    |  1 -
 net/l2tp/l2tp_debugfs.c | 15 +++++++++++++--
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index c8c4183f0f37..40261cb68e83 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -355,26 +355,6 @@ int l2tp_session_register(struct l2tp_session *session,
 }
 EXPORT_SYMBOL_GPL(l2tp_session_register);
 
-struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth)
-{
-	struct l2tp_net *pn = l2tp_pernet(net);
-	struct l2tp_tunnel *tunnel;
-	int count = 0;
-
-	rcu_read_lock_bh();
-	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
-		if (++count > nth) {
-			rcu_read_unlock_bh();
-			return tunnel;
-		}
-	}
-
-	rcu_read_unlock_bh();
-
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(l2tp_tunnel_find_nth);
-
 /*****************************************************************************
  * Receive data handling
  *****************************************************************************/
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index e4896413b2b6..c199020f8a8a 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -222,7 +222,6 @@ struct l2tp_session *l2tp_session_get(const struct net *net,
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth);
 struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 						const char *ifname);
-struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth);
 
 int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
 		       u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 72e713da4733..b8f9d45bfeb1 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -47,7 +47,11 @@ struct l2tp_dfs_seq_data {
 
 static void l2tp_dfs_next_tunnel(struct l2tp_dfs_seq_data *pd)
 {
-	pd->tunnel = l2tp_tunnel_find_nth(pd->net, pd->tunnel_idx);
+	/* Drop reference taken during previous invocation */
+	if (pd->tunnel)
+		l2tp_tunnel_dec_refcount(pd->tunnel);
+
+	pd->tunnel = l2tp_tunnel_get_nth(pd->net, pd->tunnel_idx);
 	pd->tunnel_idx++;
 }
 
@@ -96,7 +100,14 @@ static void *l2tp_dfs_seq_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void l2tp_dfs_seq_stop(struct seq_file *p, void *v)
 {
-	/* nothing to do */
+	struct l2tp_dfs_seq_data *pd = v;
+
+	if (!pd || pd == SEQ_START_TOKEN)
+		return;
+
+	/* Drop reference taken by last invocation of l2tp_dfs_next_tunnel() */
+	if (pd->tunnel)
+		l2tp_tunnel_dec_refcount(pd->tunnel);
 }
 
 static void l2tp_dfs_seq_tunnel_show(struct seq_file *m, void *v)
-- 
2.17.0

Re: [PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Thu, 12 Apr 2018 20:50:33 +0200

> l2tp_tunnel_find_nth() is unsafe: no reference is held on the returned
> tunnel, therefore it can be freed whenever the caller uses it.
> This patch defines l2tp_tunnel_get_nth() which works similarly, but
> also takes a reference on the returned tunnel. The caller then has to
> drop it after it stops using the tunnel.
> 
> Convert netlink dumps to make them safe against concurrent tunnel
> deletion.
> 
> Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

During the entire invocation of l2tp_nl_cmd_tunnel_dump(), the RTNL
mutex is held.

Therefore no tunnel configuration changes may occur and the tunnel
object will persist and is safe to access.

The netlink dump should be safe as-is.

Were you actually able to trigger a crash or KASAN warning or is
this purely from code inspection?

Re: [PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps

[Guillaume Nault]

On Fri, Apr 13, 2018 at 10:57:03AM -0400, David Miller wrote:
> From: Guillaume Nault <g.nault@alphalink.fr>
> Date: Thu, 12 Apr 2018 20:50:33 +0200
> 
> > l2tp_tunnel_find_nth() is unsafe: no reference is held on the returned
> > tunnel, therefore it can be freed whenever the caller uses it.
> > This patch defines l2tp_tunnel_get_nth() which works similarly, but
> > also takes a reference on the returned tunnel. The caller then has to
> > drop it after it stops using the tunnel.
> > 
> > Convert netlink dumps to make them safe against concurrent tunnel
> > deletion.
> > 
> > Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
> > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
> 
> During the entire invocation of l2tp_nl_cmd_tunnel_dump(), the RTNL
> mutex is held.
> 
> Therefore no tunnel configuration changes may occur and the tunnel
> object will persist and is safe to access.
> 
Yes, but only for updates done with the genl API. For L2TPv2, the
tunnel can be created by connecting a PPPOL2TP and a UDP socket.
Closing these sockets destroys the tunnel without any RTNL
synchronisation.

> The netlink dump should be safe as-is.
> 
> Were you actually able to trigger a crash or KASAN warning or is
> this purely from code inspection?
> 
Yes, I have a KASAN use-after-free for this case. I remember I saw a
few complains about stack traces in commit messages, so I've stopped
putting them there. I can paste (stripped) traces again. Just let me
know if you have any preference.

Guillaume

Re: [PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Fri, 13 Apr 2018 18:09:12 +0200

> On Fri, Apr 13, 2018 at 10:57:03AM -0400, David Miller wrote:
>> From: Guillaume Nault <g.nault@alphalink.fr>
>> Date: Thu, 12 Apr 2018 20:50:33 +0200
>> 
>> > l2tp_tunnel_find_nth() is unsafe: no reference is held on the returned
>> > tunnel, therefore it can be freed whenever the caller uses it.
>> > This patch defines l2tp_tunnel_get_nth() which works similarly, but
>> > also takes a reference on the returned tunnel. The caller then has to
>> > drop it after it stops using the tunnel.
>> > 
>> > Convert netlink dumps to make them safe against concurrent tunnel
>> > deletion.
>> > 
>> > Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
>> > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
>> 
>> During the entire invocation of l2tp_nl_cmd_tunnel_dump(), the RTNL
>> mutex is held.
>> 
>> Therefore no tunnel configuration changes may occur and the tunnel
>> object will persist and is safe to access.
>> 
> Yes, but only for updates done with the genl API. For L2TPv2, the
> tunnel can be created by connecting a PPPOL2TP and a UDP socket.
> Closing these sockets destroys the tunnel without any RTNL
> synchronisation.

Right, that's the part I missed.  Thanks for explaining.