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

[Paolo Abeni <pabeni@redhat.com>]

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