Re: [PATCH net-next v2 6/7] ipv6: convert do_ipv6_getsockopt() to sockopt_t

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] ipv6_getsockopt() now reads optlen and replaces a negative
  value with 4 before it calls do_ipv6_getsockopt().
- [Low] When the caller passes a negative optlen, ipv6_getsockopt() sets
  sopt.optlen to 4 and then always runs `put_user(sopt.optlen, optlen)`,…
- [Low] The commit message explains why sockopt_init_user() cannot be
  used by pointing to 'See discussion in the previous patch/commit.' The…
- [Low] ipv6_getsockopt() now declares `int len, err;` at function scope
  (line 1359) and then declares `int len = sopt.optlen;` again inside…

--

ipv6: convert do_ipv6_getsockopt() to sockopt_t

This commit converts do_ipv6_getsockopt(), ip6_mroute_getsockopt() and
ipv6_getsockopt_sticky() from sockptr_t to sockopt_t. ipv6_getsockopt()
now reads optlen itself and treats a negative value as 4. It builds a
user-backed sockopt_t with a new sockopt_set_user() helper and always
writes optlen back, while sol_ipv6_sockopt() builds its sockopt_t with
sockptr_to_sockopt().

> ipv6_getsockopt() cannot use sockopt_init_user() as its IPv4 sibling
> does, since that rejects a negative optlen and this level answers it as
> 4. See discussion in the previous patch/commit.

[Severity: Low]

Which commit is "the previous patch/commit"? The patch right before this
one in the series is "ipv4: convert do_ip_getsockopt() to sockopt_t", and
it does not discuss negative optlen.

The reasoning appears to be in "ipv6: treat a negative optlen as 4 in
do_ipv6_getsockopt()", which is several patches earlier. Could the message
name that commit directly?

The message also doesn't mention the new sockopt_set_user() helper. It is
added to include/linux/net.h by splitting it out of sockopt_init_user().

> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index b488391496af3..c9787579531f6 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c

[ ... ]

> @@ -998,42 +998,21 @@ static int compat_ipv6_get_msfilter(struct sock *sk, sockopt_t *opt)
>  	return 0;
>  }
>  
> -int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
> -		       sockptr_t optval, sockptr_t optlen)
> +int do_ipv6_getsockopt(struct sock *sk, int level, int optname, sockopt_t *sopt)
>  {
>  	struct ipv6_pinfo *np = inet6_sk(sk);
>  	int len;
>  	int val;
>  
>  	if (ip6_mroute_opt(optname))
> -		return ip6_mroute_getsockopt(sk, optname, optval, optlen);
> +		return ip6_mroute_getsockopt(sk, optname, sopt);
>  
> -	if (copy_from_sockptr(&len, optlen, sizeof(int)))
> -		return -EFAULT;
> -	/* Historic bug compatibility: the int options have always taken a
> -	 * negative optlen as 4, so take it as 4 everywhere.
> -	 */
> -	if (len < 0)
> -		len = 4;
> +	len = sopt->optlen;

[Severity: Medium]

Before this patch, the negative-optlen-as-4 handling ran after the
ip6_mroute_opt() dispatch, so MRT6_VERSION, MRT6_PIM and MRT6_ASSERT never
saw it. Now that the handling lives in ipv6_getsockopt(), does it also
apply to the mroute options?

ipv6_getsockopt()
    if (len < 0)
        len = 4;
    sockopt_set_user(&sopt, optval, len);
  do_ipv6_getsockopt()
    ip6_mroute_getsockopt()
        olr = min_t(int, sopt->optlen, sizeof(int));
        if (olr < 0)
            return -EINVAL;

The old ip6_mroute_getsockopt() read the raw user optlen and returned
-EINVAL for a negative value. When the caller passes -1, olr is now 4.
On a raw ICMPv6 socket, getsockopt(SOL_IPV6, MRT6_VERSION) with
optlen = -1 now succeeds, writes 4 bytes into optval and sets optlen
to 4.

The other caller, sol_ipv6_sockopt(), goes through sockptr_to_sockopt().
That already rejects a negative length, so the olr < 0 check in
ip6_mroute_getsockopt() looks unreachable now.

The IPv4 side still rejects this input. ip_getsockopt() uses
sockopt_init_user(), which returns -EINVAL for len < 0, and
ip_mroute_getsockopt() keeps its own olr < 0 check. Is the difference
between IPv4 and IPv6 intended?

The comment moved into ipv6_getsockopt() says "the int options have
always taken a negative optlen as 4", which was not true for MRT6_*.

The order of operations also changes for these options. On a socket that
isn't raw ICMPv6, the old code returned -EOPNOTSUPP without touching
optlen. Now get_user() and put_user() on optlen run first, so an
unreadable or unwritable optlen gives -EFAULT instead.

Should the mroute options skip the negative-to-4 conversion? If not,
could the commit message describe this behavior change?

[ ... ]

> @@ -1377,7 +1355,8 @@ int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
>  int ipv6_getsockopt(struct sock *sk, int level, int optname,
>  		    char __user *optval, int __user *optlen)
>  {
> -	int err;
> +	sockopt_t sopt;
> +	int len, err;
>  
>  	if (level == SOL_IP && sk->sk_type != SOCK_RAW)
>  		return ip_getsockopt(sk, level, optname, optval, optlen);
> @@ -1385,15 +1364,22 @@ int ipv6_getsockopt(struct sock *sk, int level, int optname,
>  	if (level != SOL_IPV6)
>  		return -ENOPROTOOPT;
>  
> -	err = do_ipv6_getsockopt(sk, level, optname,
> -				 USER_SOCKPTR(optval), USER_SOCKPTR(optlen));
> +	if (get_user(len, optlen))
> +		return -EFAULT;
> +	/* Historic bug compatibility: the int options have always taken a
> +	 * negative optlen as 4, so take it as 4 everywhere.
> +	 */
> +	if (len < 0)
> +		len = 4;
> +	sockopt_set_user(&sopt, optval, len);
> +
> +	err = do_ipv6_getsockopt(sk, level, optname, &sopt);
> +	if (put_user(sopt.optlen, optlen))
> +		return -EFAULT;

[Severity: Low]

When the caller passes a negative optlen, sopt.optlen is 4 at this point,
and put_user() runs whatever err is. Do failing calls now change the
caller's optlen from -1 to 4?

For example, IPV6_PATHMTU with optlen = -1 fails the
"len < sizeof(mtuinfo)" check and returns -EINVAL, and then
put_user(4, optlen) runs. The same happens for:

- -ENOTCONN from IPV6_MTU
- -EINVAL from IPV6_FLOWLABEL_MGR and MCAST_MSFILTER
- -EOPNOTSUPP and -ENOENT from the MRT6_* options
- the default -ENOPROTOOPT

In the parent commit, none of these paths wrote optlen. For MCAST_MSFILTER,
sockptr_to_sockopt() rejected a negative optlen and left optlen alone.
Earlier patches in the series described this as leaving "the caller's
optlen word unchanged".

The netfilter fallback below now gets 4 instead of the caller's original
value. If nf_getsockopt() fails, optlen has already been overwritten.

The commit message mentions the unconditional write-back. It doesn't say
that the stored value can differ from the caller's input on failure. Is
that intended?

>  #ifdef CONFIG_NETFILTER
>  	/* we need to exclude all possible ENOPROTOOPTs except default case */
>  	if (err == -ENOPROTOOPT && optname != IPV6_2292PKTOPTIONS) {
> -		int len;
> -
> -		if (get_user(len, optlen))
> -			return -EFAULT;
> +		int len = sopt.optlen;

[Severity: Low]

This isn't a bug, but this inner int len shadows the int len that this
patch adds at the top of ipv6_getsockopt(). Could it simply be
"len = sopt.optlen;"? W=2 builds with -Wshadow will warn here.

>  
>  		err = nf_getsockopt(sk, PF_INET6, optname, optval, &len);
>  		if (err >= 0)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-sockopt_expand_out_v2-v2-0-8ac08c469ecb%40debian.org