ACK: [PATCH 1/1][SRU][X] l2tp: pass tunnel pointer to ->session_create()

[Stefan Bader]

On 18.02.19 16:19, AceLan Kao wrote:
> From: Guillaume Nault <g.nault at alphalink.fr>
> 
> Using l2tp_tunnel_find() in pppol2tp_session_create() and
> l2tp_eth_create() is racy, because no reference is held on the
> returned session. These functions are only used to implement the
> ->session_create callback which is run by l2tp_nl_cmd_session_create().
> Therefore searching for the parent tunnel isn't necessary because
> l2tp_nl_cmd_session_create() already has a pointer to it and holds a
> reference.
> 
> This patch modifies ->session_create()'s prototype to directly pass the
> the parent tunnel as parameter, thus avoiding searching for it in
> pppol2tp_session_create() and l2tp_eth_create().
> 
> Since we have to touch the ->session_create() call in
> l2tp_nl_cmd_session_create(), let's also remove the useless conditional:
> we know that ->session_create isn't NULL at this point because it's
> already been checked earlier in this same function.
> 
> Finally, one might be tempted to think that the removed
> l2tp_tunnel_find() calls were harmless because they would return the
> same tunnel as the one held by l2tp_nl_cmd_session_create() anyway.
> But that tunnel might be removed and a new one created with same tunnel
> Id before the l2tp_tunnel_find() call. In this case l2tp_tunnel_find()
> would return the new tunnel which wouldn't be protected by the
> reference held by l2tp_nl_cmd_session_create().
> 
> CVE-2018-9517
> 
> Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
> Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
> Signed-off-by: Guillaume Nault <g.nault at alphalink.fr>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (backported from commit f026bc29a8e093edfbb2a77700454b285c97e8ad)
> Signed-off-by: AceLan Kao <acelan.kao at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
>  net/l2tp/l2tp_core.h    |  4 +++-
>  net/l2tp/l2tp_eth.c     | 11 +++--------
>  net/l2tp/l2tp_netlink.c |  8 ++++----
>  net/l2tp/l2tp_ppp.c     | 19 +++++++------------
>  4 files changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
> index 9cf546846edb..03602df06f4e 100644
> --- a/net/l2tp/l2tp_core.h
> +++ b/net/l2tp/l2tp_core.h
> @@ -209,7 +209,9 @@ struct l2tp_tunnel {
>  };
>  
>  struct l2tp_nl_cmd_ops {
> -	int (*session_create)(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg);
> +	int (*session_create)(struct net *net, struct l2tp_tunnel *tunnel,
> +			      u32 session_id, u32 peer_session_id,
> +			      struct l2tp_session_cfg *cfg);
>  	int (*session_delete)(struct l2tp_session *session);
>  };
>  
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index e253c26f31ac..f116d969416b 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -206,23 +206,18 @@ static void l2tp_eth_show(struct seq_file *m, void *arg)
>  }
>  #endif
>  
> -static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
> +static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
> +			   u32 session_id, u32 peer_session_id,
> +			   struct l2tp_session_cfg *cfg)
>  {
>  	struct net_device *dev;
>  	char name[IFNAMSIZ];
> -	struct l2tp_tunnel *tunnel;
>  	struct l2tp_session *session;
>  	struct l2tp_eth *priv;
>  	struct l2tp_eth_sess *spriv;
>  	int rc;
>  	struct l2tp_eth_net *pn;
>  
> -	tunnel = l2tp_tunnel_find(net, tunnel_id);
> -	if (!tunnel) {
> -		rc = -ENODEV;
> -		goto out;
> -	}
> -
>  	session = l2tp_session_find(net, tunnel, session_id);
>  	if (session) {
>  		rc = -EEXIST;
> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> index fb3248ff8b48..5a68b776a226 100644
> --- a/net/l2tp/l2tp_netlink.c
> +++ b/net/l2tp/l2tp_netlink.c
> @@ -620,10 +620,10 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>  		break;
>  	}
>  
> -	ret = -EPROTONOSUPPORT;
> -	if (l2tp_nl_cmd_ops[cfg.pw_type]->session_create)
> -		ret = (*l2tp_nl_cmd_ops[cfg.pw_type]->session_create)(net, tunnel_id,
> -			session_id, peer_session_id, &cfg);
> +	ret = l2tp_nl_cmd_ops[cfg.pw_type]->session_create(net, tunnel,
> +							   session_id,
> +							   peer_session_id,
> +							   &cfg);
>  
>  	if (ret >= 0) {
>  		session = l2tp_session_find(net, tunnel, session_id);
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index 2764c4bd072c..783da2550d7b 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -809,25 +809,20 @@ end:
>  
>  #ifdef CONFIG_L2TP_V3
>  
> -/* Called when creating sessions via the netlink interface.
> - */
> -static int pppol2tp_session_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
> +/* Called when creating sessions via the netlink interface. */
> +static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
> +				   u32 session_id, u32 peer_session_id,
> +				   struct l2tp_session_cfg *cfg)
>  {
>  	int error;
> -	struct l2tp_tunnel *tunnel;
>  	struct l2tp_session *session;
>  	struct pppol2tp_session *ps;
>  
> -	tunnel = l2tp_tunnel_find(net, tunnel_id);
> -
> -	/* Error if we can't find the tunnel */
> -	error = -ENOENT;
> -	if (tunnel == NULL)
> -		goto out;
> -
>  	/* Error if tunnel socket is not prepped */
> -	if (tunnel->sock == NULL)
> +	if (!tunnel->sock) {
> +		error = -ENOENT;
>  		goto out;
> +	}
>  
>  	/* Check that this session doesn't already exist */
>  	error = -EEXIST;
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20190222/892848cd/attachment-0001.sig>