Date:   Thu, 2 Feb 2023 17:43:49 +0100
From:   Guillaume Nault <gnault@redhat.com>
To:     Shigeru Yoshida <syoshida@redhat.com>
Cc:     jchapman@katalix.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] l2tp: Avoid possible recursive deadlock in
 l2tp_tunnel_register()
Message-ID: <Y9voRRiiWK/V7WQD@debian>
References: <20230130154438.1373750-1-syoshida@redhat.com>
 <Y9f4eAhcJXhh0+c2@debian>
 <Y9qItT82LcJdJVlF@kernel-devel>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9qItT82LcJdJVlF@kernel-devel>
Precedence: bulk

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
> 
> > 
>