On Wed, Apr 14, 2021 at 2:14 PM Sishuai Gong <sish...@purdue.edu> wrote: > > 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 <sish...@purdue.edu> > Reported-by: Sishuai Gong <sish...@purdue.edu> > --- > 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.
