netdev - Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and sock_release() without sk_callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Fri, 18 Nov 2022 23:10:38 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Tom Parkin <tparkin@...alix.com>,
        syzbot <syzbot+703d9e154b3b58277261@...kaller.appspotmail.com>,
        netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        Haowei Yan <g1042620637@...il.com>
Subject: Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and
 sock_release() without sk_callback_lock

On Fri, Nov 18, 2022 at 06:50 PM +01, Jakub Sitnicki wrote:

[...]

> But, I also have an alternative idea - instead of pulling the function
> call that might sleep out of the critical section, I think we could make
> the critical section much shorter by rearranging the tunnel
> initialization code slightly. That is, a change like below.

[...]

> --8<--
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 754fdda8a5f5..07454c0418e3 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1474,11 +1474,15 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  	}
>  
>  	sk = sock->sk;
> -	write_lock(&sk->sk_callback_lock);
> +	write_lock_bh(&sk->sk_callback_lock);
>  
>  	ret = l2tp_validate_socket(sk, net, tunnel->encap);
>  	if (ret < 0)
>  		goto err_sock;
> +	if (tunnel->encap != L2TP_ENCAPTYPE_UDP)
> +		rcu_assign_sk_user_data(sk, tunnel);

sk_user_data needs to be reset back to NULL if we bail out when the
tunnel already exists. Will add that and turn it into a patch tomorrow.

> +
> +	write_unlock_bh(&sk->sk_callback_lock);
>  
>  	tunnel->l2tp_net = net;
>  	pn = l2tp_pernet(net);
> @@ -1507,8 +1511,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  		};
>  
>  		setup_udp_tunnel_sock(net, sock, &udp_cfg);
> -	} else {
> -		rcu_assign_sk_user_data(sk, tunnel);
>  	}
>  
>  	tunnel->old_sk_destruct = sk->sk_destruct;
> @@ -1522,7 +1524,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  	if (tunnel->fd >= 0)
>  		sockfd_put(sock);
>  
> -	write_unlock(&sk->sk_callback_lock);
>  	return 0;
>  
>  err_sock:
> @@ -1530,8 +1531,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  		sock_release(sock);
>  	else
>  		sockfd_put(sock);
> -
> -	write_unlock(&sk->sk_callback_lock);
>  err:
>  	return ret;
>  }