l2tp: remove unsafe calls to l2tp_tunnel_find_nth()
Subsystems (3)
cover l2tp: remove unsafe calls to l2tp_tunnel_find_nth()
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(-)
Signature · 1 line
2.17.0
PATCH 1/3 l2tp: hold reference on tunnels in netlink dumps
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.
--- 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(-)
net/l2tp/l2tp_core.c +20 -0
@@ -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,
net/l2tp/l2tp_core.h +2 -0
@@ -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,
net/l2tp/l2tp_netlink.c +8 -3
@@ -487,14 +487,17 @@ static int l2tp_nl_cmd_tunnel_dump(struct sk_buff *skb, struct netlink_callbackstruct 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_callbackfor (;;) {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);
Signature · 1 line
2.17.0
David Miller
maintainerRe: [PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps
From: Guillaume Nault <[email protected]>
Date: Thu, 12 Apr 2018 20:50:33 +0200
Quoted 11 lines
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 definesl2tp_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 <[email protected]>
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
Quoted 12 lines · David Miller
From: Guillaume Nault <[email protected]>
Date: Thu, 12 Apr 2018 20:50:33 +0200Quoted 9 lines
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 definesl2tp_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.
Quoted 9 lines
Quoted 2 lines
Fixes 309795f4bec2 ("l2tp: Add netlink control API for L2TP")Signed-off-by Guillaume Nault <[email protected]>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.
Quoted 5 lines
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
David Miller
maintainerRe: [PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps
From: Guillaume Nault <[email protected]>
Date: Fri, 13 Apr 2018 18:09:12 +0200
Quoted 19 lines · David Miller
Quoted 18 lines
From: Guillaume Nault <[email protected]>
Date: Thu, 12 Apr 2018 20:50:33 +0200Quoted 11 lines
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 definesl2tp_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 <[email protected]>During the entire invocation of
l2tp_nl_cmd_tunnel_dump(), the RTNL mutex is held.
Quoted 7 lines
Quoted 3 lines
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.
PATCH 2/3 l2tp: hold reference on tunnels printed in pppol2tp proc file
In reply to Guillaume NaultUse 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.
--- net/l2tp/l2tp_ppp.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
net/l2tp/l2tp_ppp.c +17 -7
@@ -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)
Signature · 1 line
2.17.0
PATCH 3/3 l2tp: hold reference on tunnels printed in l2tp/tunnels debugfs file
In reply to Guillaume NaultUse 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().
--- 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(-)
net/l2tp/l2tp_core.c +0 -20
@@ -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 *****************************************************************************/
net/l2tp/l2tp_core.h +0 -1
@@ -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,
net/l2tp/l2tp_debugfs.c +13 -2
@@ -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)
Signature · 1 line
2.17.0