Re: [PATCH] net: fix a concurrency bug in l2tp_tunnel_register()
From: Cong Wang <hidden>
Date: 2021-04-15 04:10:36
On Wed, Apr 14, 2021 at 2:14 PM Sishuai Gong [off-list ref] wrote:
quoted hunk
l2tp_tunnel_register() registers a tunnel without fully initializing its attribute. This can allow another kernel thread running l2tp_xmit_core() to access the uninitialized data and then cause a kernel NULL pointer dereference error, as shown below. Thread 1 Thread 2 //l2tp_tunnel_register() list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list); //pppol2tp_connect() tunnel = l2tp_tunnel_get(sock_net(sk), info.tunnel_id); // Fetch the new tunnel ... //l2tp_xmit_core() struct sock *sk = tunnel->sock; ... bh_lock_sock(sk); //Bug happens tunnel->sock = sk; Fix this bug by initializing tunnel->sock before adding the tunnel into l2tp_tunnel_list. Signed-off-by: Sishuai Gong <redacted> Reported-by: Sishuai Gong <redacted> --- net/l2tp/l2tp_core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 203890e378cb..d61c9879076e 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c@@ -1478,6 +1478,10 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, tunnel->l2tp_net = net; pn = l2tp_pernet(net); + sk = sock->sk; + sock_hold(sk); + tunnel->sock = sk; +
I think you have to put this sock refcnt on the -EEXIST path after moving it up. Thanks.