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

[Jakub Kicinski <kuba@kernel.org>]

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