diff mbox series

l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

Message ID 20230130154438.1373750-1-syoshida@redhat.com
State New
Headers
Series l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register() |

Commit Message

Shigeru Yoshida Jan. 30, 2023, 3:44 p.m. UTC 
  When a file descriptor of pppol2tp socket is passed as file descriptor
of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
This situation is reproduced by the following program:

int main(void)
{
	int sock;
	struct sockaddr_pppol2tp addr;

	sock = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
	if (sock < 0) {
		perror("socket");
		return 1;
	}

	addr.sa_family = AF_PPPOX;
	addr.sa_protocol = PX_PROTO_OL2TP;
	addr.pppol2tp.pid = 0;
	addr.pppol2tp.fd = sock;
	addr.pppol2tp.addr.sin_family = PF_INET;
	addr.pppol2tp.addr.sin_port = htons(0);
	addr.pppol2tp.addr.sin_addr.s_addr = inet_addr("192.168.0.1");
	addr.pppol2tp.s_tunnel = 1;
	addr.pppol2tp.s_session = 0;
	addr.pppol2tp.d_tunnel = 0;
	addr.pppol2tp.d_session = 0;

	if (connect(sock, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
		perror("connect");
		return 1;
	}

	return 0;
}

This program causes the following lockdep warning:

 ============================================
 WARNING: possible recursive locking detected
 6.2.0-rc5-00205-gc96618275234 #56 Not tainted
 --------------------------------------------
 repro/8607 is trying to acquire lock:
 ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: l2tp_tunnel_register+0x2b7/0x11c0

 but task is already holding lock:
 ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: pppol2tp_connect+0xa82/0x1a30

 other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(sk_lock-AF_PPPOX);
   lock(sk_lock-AF_PPPOX);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

 1 lock held by repro/8607:
  #0: ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: pppol2tp_connect+0xa82/0x1a30

 stack backtrace:
 CPU: 0 PID: 8607 Comm: repro Not tainted 6.2.0-rc5-00205-gc96618275234 #56
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
 Call Trace:
  <TASK>
  dump_stack_lvl+0x100/0x178
  __lock_acquire.cold+0x119/0x3b9
  ? lockdep_hardirqs_on_prepare+0x410/0x410
  lock_acquire+0x1e0/0x610
  ? l2tp_tunnel_register+0x2b7/0x11c0
  ? lock_downgrade+0x710/0x710
  ? __fget_files+0x283/0x3e0
  lock_sock_nested+0x3a/0xf0
  ? l2tp_tunnel_register+0x2b7/0x11c0
  l2tp_tunnel_register+0x2b7/0x11c0
  ? sprintf+0xc4/0x100
  ? l2tp_tunnel_del_work+0x6b0/0x6b0
  ? debug_object_deactivate+0x320/0x320
  ? lockdep_init_map_type+0x16d/0x7a0
  ? lockdep_init_map_type+0x16d/0x7a0
  ? l2tp_tunnel_create+0x2bf/0x4b0
  ? l2tp_tunnel_create+0x3c6/0x4b0
  pppol2tp_connect+0x14e1/0x1a30
  ? pppol2tp_put_sk+0xd0/0xd0
  ? aa_sk_perm+0x2b7/0xa80
  ? aa_af_perm+0x260/0x260
  ? bpf_lsm_socket_connect+0x9/0x10
  ? pppol2tp_put_sk+0xd0/0xd0
  __sys_connect_file+0x14f/0x190
  __sys_connect+0x133/0x160
  ? __sys_connect_file+0x190/0x190
  ? lockdep_hardirqs_on+0x7d/0x100
  ? ktime_get_coarse_real_ts64+0x1b7/0x200
  ? ktime_get_coarse_real_ts64+0x147/0x200
  ? __audit_syscall_entry+0x396/0x500
  __x64_sys_connect+0x72/0xb0
  do_syscall_64+0x38/0xb0
  entry_SYSCALL_64_after_hwframe+0x63/0xcd

This patch fixes the issue by returning error when a pppol2tp socket
itself is passed.

Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/l2tp/l2tp_ppp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Guillaume Nault Jan. 30, 2023, 5:03 p.m. UTC | #1 
On Tue, Jan 31, 2023 at 12:44:38AM +0900, Shigeru Yoshida wrote:
> This patch fixes the issue by returning error when a pppol2tp socket
> itself is passed.

Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()")

> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
>  net/l2tp/l2tp_ppp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index db2e584c625e..88d1a339500b 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -702,11 +702,14 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
>  			struct l2tp_tunnel_cfg tcfg = {
>  				.encap = L2TP_ENCAPTYPE_UDP,
>  			};
> +			int dummy = 0;

There's no need to initialise dummy here. This is just confusing.
We could even do without any extra variable and reuse error in
sockfd_lookup().

>  			/* Prevent l2tp_tunnel_register() from trying to set up
> -			 * a kernel socket.
> +			 * a kernel socket.  Also, prevent l2tp_tunnel_register()
> +			 * from trying to use pppol2tp socket itself.
>  			 */
> -			if (info.fd < 0) {
> +			if (info.fd < 0 ||
> +			    sock == sockfd_lookup(info.fd, &dummy)) {
>  				error = -EBADF;
>  				goto end;
>  			}

That should work, but the real problem is calling l2tp_tunnel_register()
under lock_sock(). We should instead get/create the tunnel before
locking the pppol2tp socket.
Shigeru Yoshida Feb. 1, 2023, 3:43 p.m. UTC | #2 
Hi Guillaume,

On Mon, Jan 30, 2023 at 06:03:52PM +0100, Guillaume Nault wrote:
> On Tue, Jan 31, 2023 at 12:44:38AM +0900, Shigeru Yoshida wrote:
> > This patch fixes the issue by returning error when a pppol2tp socket
> > itself is passed.
> 
> Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()")
> 
> > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > ---
> >  net/l2tp/l2tp_ppp.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> > index db2e584c625e..88d1a339500b 100644
> > --- a/net/l2tp/l2tp_ppp.c
> > +++ b/net/l2tp/l2tp_ppp.c
> > @@ -702,11 +702,14 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
> >  			struct l2tp_tunnel_cfg tcfg = {
> >  				.encap = L2TP_ENCAPTYPE_UDP,
> >  			};
> > +			int dummy = 0;
> 
> There's no need to initialise dummy here. This is just confusing.
> We could even do without any extra variable and reuse error in
> sockfd_lookup().
> 
> >  			/* Prevent l2tp_tunnel_register() from trying to set up
> > -			 * a kernel socket.
> > +			 * a kernel socket.  Also, prevent l2tp_tunnel_register()
> > +			 * from trying to use pppol2tp socket itself.
> >  			 */
> > -			if (info.fd < 0) {
> > +			if (info.fd < 0 ||
> > +			    sock == sockfd_lookup(info.fd, &dummy)) {
> >  				error = -EBADF;
> >  				goto end;
> >  			}
> 
> That should work, but the real problem is calling l2tp_tunnel_register()
> under lock_sock(). We should instead get/create the tunnel before
> locking the pppol2tp socket.

Thank you so much for your comment, and sorry for the late response.

Do you mean we can call l2tp_tunnel_register() without pppol2tp socket
lock?  I've read the source code of pppol2tp_connect(), but I'm not
sure why pppol2tp socket is locked at the beginning of this function.

If we can call l2tp_tunnel_register() without pppol2tp socket lock, I
think we can move lock_sock() after l2tp_tunnel_register().

Thanks,
Shigeru

>
Guillaume Nault Feb. 2, 2023, 4:43 p.m. UTC | #3 
On Thu, Feb 02, 2023 at 12:43:49AM +0900, Shigeru Yoshida wrote:
> Hi Guillaume,
> 
> On Mon, Jan 30, 2023 at 06:03:52PM +0100, Guillaume Nault wrote:
> > On Tue, Jan 31, 2023 at 12:44:38AM +0900, Shigeru Yoshida wrote:
> > > This patch fixes the issue by returning error when a pppol2tp socket
> > > itself is passed.
> > 
> > Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()")
> > 
> > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > ---
> > >  net/l2tp/l2tp_ppp.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> > > index db2e584c625e..88d1a339500b 100644
> > > --- a/net/l2tp/l2tp_ppp.c
> > > +++ b/net/l2tp/l2tp_ppp.c
> > > @@ -702,11 +702,14 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
> > >  			struct l2tp_tunnel_cfg tcfg = {
> > >  				.encap = L2TP_ENCAPTYPE_UDP,
> > >  			};
> > > +			int dummy = 0;
> > 
> > There's no need to initialise dummy here. This is just confusing.
> > We could even do without any extra variable and reuse error in
> > sockfd_lookup().
> > 
> > >  			/* Prevent l2tp_tunnel_register() from trying to set up
> > > -			 * a kernel socket.
> > > +			 * a kernel socket.  Also, prevent l2tp_tunnel_register()
> > > +			 * from trying to use pppol2tp socket itself.
> > >  			 */
> > > -			if (info.fd < 0) {
> > > +			if (info.fd < 0 ||
> > > +			    sock == sockfd_lookup(info.fd, &dummy)) {
> > >  				error = -EBADF;
> > >  				goto end;
> > >  			}
> > 
> > That should work, but the real problem is calling l2tp_tunnel_register()
> > under lock_sock(). We should instead get/create the tunnel before
> > locking the pppol2tp socket.
> 
> Thank you so much for your comment, and sorry for the late response.
> 
> Do you mean we can call l2tp_tunnel_register() without pppol2tp socket
> lock?

Yes. At this point, we're creating a new tunnel which is independant
from the pppol2tp socket.

> I've read the source code of pppol2tp_connect(), but I'm not
> sure why pppol2tp socket is locked at the beginning of this function.
> If we can call l2tp_tunnel_register() without pppol2tp socket lock, I
> think we can move lock_sock() after l2tp_tunnel_register().

Here are a few more details to be sure we're on the same page.

Locking the pppol2tp socket remains necessary since we access and
modify some of its protected attributes. But we can fetch or create
the tunnel before working on the socket. For this, the only information
we need to get from the socket is its netns. And calling sock_net(sk)
without holding the socket lock is fine because user space sockets
can't have their netns modified after initialisation.

So the code for retrieving or creating the tunnel can be moved before
the lock_sock(sk) call in pppol2tp_register(). Just make sure to adjust
the error path accordingly. Also, a helper function might help to make
the code more readable.

> Thanks,
> Shigeru
> 
> > 
>
Shigeru Yoshida Feb. 7, 2023, 4:50 p.m. UTC | #4 
Hi Guillaume,

On Thu, Feb 02, 2023 at 05:43:49PM +0100, Guillaume Nault wrote:
> On Thu, Feb 02, 2023 at 12:43:49AM +0900, Shigeru Yoshida wrote:
> > Hi Guillaume,
> > 
> > On Mon, Jan 30, 2023 at 06:03:52PM +0100, Guillaume Nault wrote:
> > > On Tue, Jan 31, 2023 at 12:44:38AM +0900, Shigeru Yoshida wrote:
> > > > This patch fixes the issue by returning error when a pppol2tp socket
> > > > itself is passed.
> > > 
> > > Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()")
> > > 
> > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > > ---
> > > >  net/l2tp/l2tp_ppp.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> > > > index db2e584c625e..88d1a339500b 100644
> > > > --- a/net/l2tp/l2tp_ppp.c
> > > > +++ b/net/l2tp/l2tp_ppp.c
> > > > @@ -702,11 +702,14 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
> > > >  			struct l2tp_tunnel_cfg tcfg = {
> > > >  				.encap = L2TP_ENCAPTYPE_UDP,
> > > >  			};
> > > > +			int dummy = 0;
> > > 
> > > There's no need to initialise dummy here. This is just confusing.
> > > We could even do without any extra variable and reuse error in
> > > sockfd_lookup().
> > > 
> > > >  			/* Prevent l2tp_tunnel_register() from trying to set up
> > > > -			 * a kernel socket.
> > > > +			 * a kernel socket.  Also, prevent l2tp_tunnel_register()
> > > > +			 * from trying to use pppol2tp socket itself.
> > > >  			 */
> > > > -			if (info.fd < 0) {
> > > > +			if (info.fd < 0 ||
> > > > +			    sock == sockfd_lookup(info.fd, &dummy)) {
> > > >  				error = -EBADF;
> > > >  				goto end;
> > > >  			}
> > > 
> > > That should work, but the real problem is calling l2tp_tunnel_register()
> > > under lock_sock(). We should instead get/create the tunnel before
> > > locking the pppol2tp socket.
> > 
> > Thank you so much for your comment, and sorry for the late response.
> > 
> > Do you mean we can call l2tp_tunnel_register() without pppol2tp socket
> > lock?
> 
> Yes. At this point, we're creating a new tunnel which is independant
> from the pppol2tp socket.
> 
> > I've read the source code of pppol2tp_connect(), but I'm not
> > sure why pppol2tp socket is locked at the beginning of this function.
> > If we can call l2tp_tunnel_register() without pppol2tp socket lock, I
> > think we can move lock_sock() after l2tp_tunnel_register().
> 
> Here are a few more details to be sure we're on the same page.
> 
> Locking the pppol2tp socket remains necessary since we access and
> modify some of its protected attributes. But we can fetch or create
> the tunnel before working on the socket. For this, the only information
> we need to get from the socket is its netns. And calling sock_net(sk)
> without holding the socket lock is fine because user space sockets
> can't have their netns modified after initialisation.
> 
> So the code for retrieving or creating the tunnel can be moved before
> the lock_sock(sk) call in pppol2tp_register(). Just make sure to adjust
> the error path accordingly. Also, a helper function might help to make
> the code more readable.

Thank you so much for the detailed explanation.  I really appreciate.
I'll think about it further, and try to prepare v2 patch.

Thanks,
Shigeru

> 
> > Thanks,
> > Shigeru
> > 
> > > 
> > 
>
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index db2e584c625e..88d1a339500b 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -702,11 +702,14 @@  static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 			struct l2tp_tunnel_cfg tcfg = {
 				.encap = L2TP_ENCAPTYPE_UDP,
 			};
+			int dummy = 0;
 
 			/* Prevent l2tp_tunnel_register() from trying to set up
-			 * a kernel socket.
+			 * a kernel socket.  Also, prevent l2tp_tunnel_register()
+			 * from trying to use pppol2tp socket itself.
 			 */
-			if (info.fd < 0) {
+			if (info.fd < 0 ||
+			    sock == sockfd_lookup(info.fd, &dummy)) {
 				error = -EBADF;
 				goto end;
 			}