[PATCH 1/1] l2tp: pppol2tp: hold reference to session in pppol2tp_ioctl()

[PATCH 1/1] l2tp: pppol2tp: hold reference to session in pppol2tp_ioctl()

[Lee Jones]

pppol2tp_ioctl() read sock->sk->sk_user_data directly without any locks
or reference counting.  If a controllable sleep was induced during
copy_from_user() (e.g. via a userfaultfd page fault sleep), a concurrent
socket close could trigger pppol2tp_session_close() asynchronously.  This
frees the l2tp_session structure via the l2tp_session_del_work workqueue.
Upon resuming, the ioctl thread dereferences the stale session pointer,
resulting in a Use-After-Free (UAF).

Fix this by securely fetching the session reference using the RCU-safe,
refcounted helper pppol2tp_sock_to_session(sk) on entry.  This locks the
session's refcount across the sleep.  We structured the function to exit
via standard err breaks, guaranteeing that l2tp_session_put() is cleanly
called on all return paths to drop the reference.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 net/l2tp/l2tp_ppp.c | 72 ++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 99d6582f41de..9a2243a32ceb 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1045,64 +1045,57 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
 {
 	struct pppol2tp_ioc_stats stats;
 	struct l2tp_session *session;
+	int err = 0;
+
+	session = pppol2tp_sock_to_session(sock->sk);
+	if (!session)
+		return -ENOTCONN;
 
 	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;
-
 		/* Not defined for tunnels */
-		if (!session->session_id && !session->peer_session_id)
-			return -ENOSYS;
+		if (!session->session_id && !session->peer_session_id) {
+			err = -ENOSYS;
+			break;
+		}
 
-		if (put_user(0, (int __user *)arg))
-			return -EFAULT;
+		if (put_user(0, (int __user *)arg)) {
+			err = -EFAULT;
+			break;
+		}
 		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;
-
 		/* Not defined for tunnels */
-		if (!session->session_id && !session->peer_session_id)
-			return -ENOSYS;
+		if (!session->session_id && !session->peer_session_id) {
+			err = -ENOSYS;
+			break;
+		}
 
-		if (!access_ok((int __user *)arg, sizeof(int)))
-			return -EFAULT;
+		if (!access_ok((int __user *)arg, sizeof(int))) {
+			err = -EFAULT;
+			break;
+		}
 		break;
 
 	case PPPIOCGL2TPSTATS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
-
-		if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
-			return -EBADF;
-
 		/* Session 0 represents the parent tunnel */
 		if (!session->session_id && !session->peer_session_id) {
 			u32 session_id;
-			int err;
 
 			if (copy_from_user(&stats, (void __user *)arg,
-					   sizeof(stats)))
-				return -EFAULT;
+					   sizeof(stats))) {
+				err = -EFAULT;
+				break;
+			}
 
 			session_id = stats.session_id;
 			err = pppol2tp_tunnel_copy_stats(&stats,
 							 session->tunnel);
 			if (err < 0)
-				return err;
+				break;
 
 			stats.session_id = session_id;
 		} else {
@@ -1112,15 +1105,20 @@ 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;
+			break;
+		}
 		break;
 
 	default:
-		return -ENOIOCTLCMD;
+		err = -ENOIOCTLCMD;
+		break;
 	}
 
-	return 0;
+	l2tp_session_put(session);
+
+	return err;
 }
 
 /*****************************************************************************
-- 
2.54.0.631.ge1b05301d1-goog

Re: [PATCH 1/1] l2tp: pppol2tp: hold reference to session in pppol2tp_ioctl()

[Jakub Kicinski]

On Wed, 20 May 2026 14:48:36 +0100 Lee Jones wrote:
> pppol2tp_ioctl() read sock->sk->sk_user_data directly without any locks
> or reference counting.  If a controllable sleep was induced during
> copy_from_user() (e.g. via a userfaultfd page fault sleep), a concurrent
> socket close could trigger pppol2tp_session_close() asynchronously.  This
> frees the l2tp_session structure via the l2tp_session_del_work workqueue.
> Upon resuming, the ioctl thread dereferences the stale session pointer,
> resulting in a Use-After-Free (UAF).
> 
> Fix this by securely fetching the session reference using the RCU-safe,
> refcounted helper pppol2tp_sock_to_session(sk) on entry.  This locks the
> session's refcount across the sleep.  We structured the function to exit
> via standard err breaks, guaranteeing that l2tp_session_put() is cleanly
> called on all return paths to drop the reference.

Since this is a fix we will need this reposted with a Fixes tag, please.
-- 
pw-bot: cr

Re: [PATCH 1/1] l2tp: pppol2tp: hold reference to session in pppol2tp_ioctl()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
l2tp: pppol2tp: hold reference to session in pppol2tp_ioctl()

This patch addresses a use-after-free in pppol2tp_ioctl() by replacing
the direct sk_user_data dereference with the refcounted helper
pppol2tp_sock_to_session(), and refactors the function to drop the
reference on a single exit path.

> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index 99d6582f41de..9a2243a32ceb 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -1045,64 +1045,57 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
>  {
>  	struct pppol2tp_ioc_stats stats;
>  	struct l2tp_session *session;
> +	int err = 0;
> +
> +	session = pppol2tp_sock_to_session(sock->sk);
> +	if (!session)
> +		return -ENOTCONN;

[Medium]
Does this change the behaviour of unknown ioctl commands when the
socket is not yet connected?

Previously, the default arm of the switch returned -ENOIOCTLCMD without
ever consulting sk_user_data:

	default:
		return -ENOIOCTLCMD;

In sock_do_ioctl(), -ENOIOCTLCMD is the sentinel that lets the generic
dispatcher fall through to handlers like dev_ioctl() for SIOCGIF*-style
and similar generic socket ioctls:

	if (err != -ENOIOCTLCMD)
		return err;

After the patch, any cmd at all on a pppol2tp socket whose sk_user_data
has not been bound by pppol2tp_connect() now fails up front with
-ENOTCONN, including the generic ioctls that previously fell through
the default arm.

Would it be worth either keeping the per-case session lookup, or
mapping the no-session case onto the default arm so unknown cmds keep
returning -ENOIOCTLCMD?  The commit message describes this as a UAF
fix and does not mention this user-visible change.

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

[Low]
Is the loss of the explicit -EBADF magic-check return path intended?

Each cmd arm previously had:

	if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
		return -EBADF;

pppol2tp_sock_to_session() does perform a magic check, but only as
WARN_ON_ONCE() and it still returns the (possibly corrupted) session
pointer:

	WARN_ON_ONCE(session->magic != L2TP_SESSION_MAGIC);
	...
	return session;

So on a magic mismatch the function will now proceed to dereference
session->session_id, session->tunnel, etc. instead of bailing out with
-EBADF, and the warning is reduced from per-call to once per boot.

The commit message frames the change as purely a refcount/UAF fix and
does not mention the loss of this defensive path; was that
intentional?

>  		/* Not defined for tunnels */
> -		if (!session->session_id && !session->peer_session_id)
> -			return -ENOSYS;
> +		if (!session->session_id && !session->peer_session_id) {
> +			err = -ENOSYS;
> +			break;
> +		}
[ ... ]
-- 
pw-bot: cr