[PATCH] net: fix a concurrency bug in l2tp_tunnel_register()

[PATCH] net: fix a concurrency bug in l2tp_tunnel_register()

[Gong, Sishuai]

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);
           //Null pointer error happens
tunnel->sock = sk;

Fix this bug by initializing tunnel->sock before adding the
tunnel into l2tp_tunnel_list.

Reviewed-by: Cong Wang <cong.wang@bytedance.com>
Signed-off-by: Sishuai Gong <sishuai@purdue.edu>
Reported-by: Sishuai Gong <sishuai@purdue.edu>
---
net/l2tp/l2tp_core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 203890e378cb..8eb805ee18d4 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1478,11 +1478,15 @@ 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;
+
	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
	list_for_each_entry(tunnel_walk, &pn->l2tp_tunnel_list, list) {
		if (tunnel_walk->tunnel_id == tunnel->tunnel_id) {
			spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
-
+			sock_put(sk);
			ret = -EEXIST;
			goto err_sock;
		}
@@ -1490,10 +1494,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
	list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list);
	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);

-	sk = sock->sk;
-	sock_hold(sk);
-	tunnel->sock = sk;
-
	if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
		struct udp_tunnel_sock_cfg udp_cfg = {
			.sk_user_data = tunnel,
-- 
2.17.1

Re: [PATCH] net: fix a concurrency bug in l2tp_tunnel_register()

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On  Tue, Apr 27, 2021 at 15:04:24 +0000, Gong, Sishuai 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);
>            //Null pointer error happens
> tunnel->sock = sk;
> 
> Fix this bug by initializing tunnel->sock before adding the
> tunnel into l2tp_tunnel_list.
> 
> Reviewed-by: Cong Wang <cong.wang@bytedance.com>
> Signed-off-by: Sishuai Gong <sishuai@purdue.edu>
> Reported-by: Sishuai Gong <sishuai@purdue.edu>
> ---

It came through this time.  Mysterious.

Anyway, thank you for reposting.  This looks good to me :-)

-- 
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: fix a concurrency bug in l2tp_tunnel_register()

[Jakub Kicinski]

On Tue, 27 Apr 2021 15:04:24 +0000 Gong, Sishuai 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);
>            //Null pointer error happens
> tunnel->sock = sk;
> 
> Fix this bug by initializing tunnel->sock before adding the
> tunnel into l2tp_tunnel_list.
> 
> Reviewed-by: Cong Wang <cong.wang@bytedance.com>
> Signed-off-by: Sishuai Gong <sishuai@purdue.edu>
> Reported-by: Sishuai Gong <sishuai@purdue.edu>

Thanks for the patch! Unfortunately the patch in the email is
corrupted, looks like something removed spaces at the start of 
the lines which are expected in patches, e.g.:

»       pn·=·l2tp_pernet(net);$
$
+»      sk·=·sock->sk;$

Should have been:

 »      pn·=·l2tp_pernet(net);$
 $
+»      sk·=·sock->sk;$

Could you try to resend once more with git-send-email or via 
a different mail server?

Re: [PATCH] net: fix a concurrency bug in l2tp_tunnel_register()

[Jakub Kicinski]

On Wed, 28 Apr 2021 12:45:34 -0700 Jakub Kicinski wrote:
> On Tue, 27 Apr 2021 15:04:24 +0000 Gong, Sishuai 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);
> >            //Null pointer error happens
> > tunnel->sock = sk;
> > 
> > Fix this bug by initializing tunnel->sock before adding the
> > tunnel into l2tp_tunnel_list.
> > 
> > Reviewed-by: Cong Wang <cong.wang@bytedance.com>
> > Signed-off-by: Sishuai Gong <sishuai@purdue.edu>
> > Reported-by: Sishuai Gong <sishuai@purdue.edu>  
> 
> Thanks for the patch! Unfortunately the patch in the email is
> corrupted, looks like something removed spaces at the start of 
> the lines which are expected in patches, e.g.:
> 
> »       pn·=·l2tp_pernet(net);$
> $
> +»      sk·=·sock->sk;$
> 
> Should have been:
> 
>  »      pn·=·l2tp_pernet(net);$
>  $
> +»      sk·=·sock->sk;$
> 
> Could you try to resend once more with git-send-email or via 
> a different mail server?

Ignore me, this patch has already been merged.

Re: [PATCH] net: fix a concurrency bug in l2tp_tunnel_register()

[Cong Wang]

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