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

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

[Cong Wang]

On Wed, Apr 21, 2021 at 12:25 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);
>             //Null pointer error 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>

Reviewed-by: Cong Wang <cong.wang@bytedance.com>

Thanks.

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

[Tom Parkin]

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

On  Fri, Apr 23, 2021 at 10:38:45 -0700, Cong Wang wrote:
> On Wed, Apr 21, 2021 at 12:25 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);
> >             //Null pointer error 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>
> 
> Reviewed-by: Cong Wang <cong.wang@bytedance.com>
> 
> Thanks.

For some reason I've not been seeing these patches, just the replies.
I can't see it on lore.kernel.org either, unless I'm missing something
obvious.

Have the original mails cc'd netdev?

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

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

[Tom Parkin]

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

On  Mon, Apr 26, 2021 at 13:04:44 +0000, Gong, Sishuai wrote:
>    On Apr 26, 2021, at 4:59 AM, Tom Parkin <tparkin@katalix.com> wrote:
>      For some reason I've not been seeing these patches, just the replies.
>      I can't see it on lore.kernel.org either, unless I'm missing something
>      obvious.
> 
>      Have the original mails cc'd netdev?
> 
>    I also noticed this problem. I CCed netdev@vger.kernel.org but 
>    this message didn’t show up in the maillist. Actually, I also
>    CCed you in the original email but it looks like only Cong Wang
>    could see it.
>    Should I re-send this email?
>    Thanks.

If you CC'd me in the first place and I didn't see it that's strange.
I got your initial "bug report" mail :-|

From the perspective of submitting the patch, I have no problem with
it being applied on the strength of Cong Wang's review.

Thanks for working on this.

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