[PATCH net] l2tp: take a session reference in pppol2tp_ioctl()

[PATCH net] l2tp: take a session reference in pppol2tp_ioctl()

[Yiqi Sun]

pppol2tp_ioctl() reads sock->sk->sk_user_data and dereferences the
returned l2tp_session without taking a reference on it.

Since the ppp socket/session lifetime rework, session teardown runs
asynchronously and can clear sk_user_data and drop the last session
reference in parallel with ioctl(). This leaves ioctl() with a stale
session pointer and can trigger a use-after-free.

Fix this by using pppol2tp_sock_to_session() in pppol2tp_ioctl() and
dropping the session reference before returning. This matches the
existing getsockopt/setsockopt paths.

Fixes: c5cbaef992d64 ("l2tp: refactor ppp socket/session relationship")
Signed-off-by: Yiqi Sun <sunyiqixm@gmail.com>
---
 net/l2tp/l2tp_ppp.c | 88 +++++++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 34 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index ae4543d5597b..e6d7d3537180 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1042,66 +1042,79 @@ static int pppol2tp_tunnel_copy_stats(struct pppol2tp_ioc_stats *stats,
 static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
 			  unsigned long arg)
 {
+	struct sock *sk = sock->sk;
 	struct pppol2tp_ioc_stats stats;
 	struct l2tp_session *session;
+	int err;
+
+	err = -ENOTCONN;
+	if (!sk->sk_user_data)
+		goto end;
+
+	err = -EBADF;
+	session = pppol2tp_sock_to_session(sk);
+	if (!session)
+		goto end;
 
 	switch (cmd) {
 	case PPPIOCGMRU:
 	case PPPIOCGFLAGS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
-
 		if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
-			return -EBADF;
+			goto end_put_sess;
 
 		/* Not defined for tunnels */
-		if (!session->session_id && !session->peer_session_id)
-			return -ENOSYS;
+		if (!session->session_id && !session->peer_session_id) {
+			err = -ENOSYS;
+			goto end_put_sess;
+		}
 
-		if (put_user(0, (int __user *)arg))
-			return -EFAULT;
+		if (put_user(0, (int __user *)arg)) {
+			err = -EFAULT;
+			goto end_put_sess;
+		}
+		err = 0;
 		break;
 
 	case PPPIOCSMRU:
 	case PPPIOCSFLAGS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
-
 		if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
-			return -EBADF;
+			goto end_put_sess;
 
 		/* Not defined for tunnels */
-		if (!session->session_id && !session->peer_session_id)
-			return -ENOSYS;
+		if (!session->session_id && !session->peer_session_id) {
+			err = -ENOSYS;
+			goto end_put_sess;
+		}
 
-		if (!access_ok((int __user *)arg, sizeof(int)))
-			return -EFAULT;
+		if (!access_ok((int __user *)arg, sizeof(int))) {
+			err = -EFAULT;
+			goto end_put_sess;
+		}
+		err = 0;
 		break;
 
 	case PPPIOCGL2TPSTATS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
-
 		if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
-			return -EBADF;
+			goto end_put_sess;
 
 		/* Session 0 represents the parent tunnel */
 		if (!session->session_id && !session->peer_session_id) {
 			u32 session_id;
-			int err;
+			int rc;
 
 			if (copy_from_user(&stats, (void __user *)arg,
-					   sizeof(stats)))
-				return -EFAULT;
+					   sizeof(stats))) {
+				err = -EFAULT;
+				goto end_put_sess;
+			}
 
 			session_id = stats.session_id;
-			err = pppol2tp_tunnel_copy_stats(&stats,
-							 session->tunnel);
-			if (err < 0)
-				return err;
+			rc = pppol2tp_tunnel_copy_stats(&stats,
+							session->tunnel);
+			if (rc < 0) {
+				err = rc;
+				goto end_put_sess;
+			}
 
 			stats.session_id = session_id;
 		} else {
@@ -1111,15 +1124,22 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
 		stats.tunnel_id = session->tunnel->tunnel_id;
 		stats.using_ipsec = l2tp_tunnel_uses_xfrm(session->tunnel);
 
-		if (copy_to_user((void __user *)arg, &stats, sizeof(stats)))
-			return -EFAULT;
+		if (copy_to_user((void __user *)arg, &stats, sizeof(stats))) {
+			err = -EFAULT;
+			goto end_put_sess;
+		}
+		err = 0;
 		break;
 
 	default:
-		return -ENOIOCTLCMD;
+		err = -ENOIOCTLCMD;
+		break;
 	}
 
-	return 0;
+end_put_sess:
+	l2tp_session_put(session);
+end:
+	return err;
 }
 
 /*****************************************************************************
-- 
2.34.1

Re: [PATCH net] l2tp: take a session reference in pppol2tp_ioctl()

[Paolo Abeni]

On 4/4/26 3:32 PM, Yiqi Sun wrote:
> pppol2tp_ioctl() reads sock->sk->sk_user_data and dereferences the
> returned l2tp_session without taking a reference on it.
> 
> Since the ppp socket/session lifetime rework, session teardown runs
> asynchronously and can clear sk_user_data and drop the last session
> reference in parallel with ioctl(). This leaves ioctl() with a stale
> session pointer and can trigger a use-after-free.

It's not immediately obvious to me which are the actual code paths
involved, please include a more accurate description of the relevant race.

> 
> Fix this by using pppol2tp_sock_to_session() in pppol2tp_ioctl() and
> dropping the session reference before returning. This matches the
> existing getsockopt/setsockopt paths.
> 
> Fixes: c5cbaef992d64 ("l2tp: refactor ppp socket/session relationship")
> Signed-off-by: Yiqi Sun <sunyiqixm@gmail.com>
> ---
>  net/l2tp/l2tp_ppp.c | 88 +++++++++++++++++++++++++++------------------
>  1 file changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index ae4543d5597b..e6d7d3537180 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -1042,66 +1042,79 @@ static int pppol2tp_tunnel_copy_stats(struct pppol2tp_ioc_stats *stats,
>  static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
>  			  unsigned long arg)
>  {
> +	struct sock *sk = sock->sk;
>  	struct pppol2tp_ioc_stats stats;
>  	struct l2tp_session *session;
> +	int err;
> +
> +	err = -ENOTCONN;
> +	if (!sk->sk_user_data)
> +		goto end;
> +
> +	err = -EBADF;
> +	session = pppol2tp_sock_to_session(sk);
> +	if (!session)
> +		goto end;

Consolidating the checks here brings an user visible change, as
unsupported cmds previously delivered -ENOIOCTLCMD.

>  
>  	switch (cmd) {
>  	case PPPIOCGMRU:
>  	case PPPIOCGFLAGS:
> -		session = sock->sk->sk_user_data;
> -		if (!session)
> -			return -ENOTCONN;
> -
>  		if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))

This check is already present in pppol2tp_sock_to_session()

Similar chunks below.

/P

Re: [PATCH net] l2tp: take a session reference in pppol2tp_ioctl()

[Simon Horman]

On Sat, Apr 04, 2026 at 09:32:45PM +0800, Yiqi Sun wrote:
> pppol2tp_ioctl() reads sock->sk->sk_user_data and dereferences the
> returned l2tp_session without taking a reference on it.
> 
> Since the ppp socket/session lifetime rework, session teardown runs
> asynchronously and can clear sk_user_data and drop the last session
> reference in parallel with ioctl(). This leaves ioctl() with a stale
> session pointer and can trigger a use-after-free.
> 
> Fix this by using pppol2tp_sock_to_session() in pppol2tp_ioctl() and
> dropping the session reference before returning. This matches the
> existing getsockopt/setsockopt paths.
> 
> Fixes: c5cbaef992d64 ("l2tp: refactor ppp socket/session relationship")
> Signed-off-by: Yiqi Sun <sunyiqixm@gmail.com>
> ---
>  net/l2tp/l2tp_ppp.c | 88 +++++++++++++++++++++++++++------------------
>  1 file changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index ae4543d5597b..e6d7d3537180 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -1042,66 +1042,79 @@ static int pppol2tp_tunnel_copy_stats(struct pppol2tp_ioc_stats *stats,
>  static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
>  			  unsigned long arg)
>  {
> +	struct sock *sk = sock->sk;
>  	struct pppol2tp_ioc_stats stats;
>  	struct l2tp_session *session;
> +	int err;
> +
> +	err = -ENOTCONN;
> +	if (!sk->sk_user_data)
> +		goto end;

I think it would be cleaner to simply:

		return -ENOTCONN;

> +
> +	err = -EBADF;
> +	session = pppol2tp_sock_to_session(sk);
> +	if (!session)
> +		goto end;

And, similarly here.

...

> @@ -1111,15 +1124,22 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
>  		stats.tunnel_id = session->tunnel->tunnel_id;
>  		stats.using_ipsec = l2tp_tunnel_uses_xfrm(session->tunnel);
>  
> -		if (copy_to_user((void __user *)arg, &stats, sizeof(stats)))
> -			return -EFAULT;
> +		if (copy_to_user((void __user *)arg, &stats, sizeof(stats))) {
> +			err = -EFAULT;
> +			goto end_put_sess;
> +		}
> +		err = 0;
>  		break;
>  
>  	default:
> -		return -ENOIOCTLCMD;
> +		err = -ENOIOCTLCMD;

I would suggest a goto here.

> +		break;
>  	}
>  

And setting err = 0 here, rather than in multiple places above.

> -	return 0;
> +end_put_sess:

I think "out_put_session" would be a slightly better name for this label.

> +	l2tp_session_put(session);
> +end:
> +	return err;
>  }
>  
>  /*****************************************************************************
> -- 
> 2.34.1
>