Re: [PATCH net-next 1/6] ipv6: reject a negative optlen in do_ipv6_getsockopt()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message misdescribes how a negative optlen behaved
  before the patch.
- [Low] The commit message frames the change as a standalone bug fix
  ("the fix stands on its own here") for user-visible behaviour, but it…

--

ipv6: reject a negative optlen in do_ipv6_getsockopt()

This commit makes do_ipv6_getsockopt() return -EINVAL when the optlen
read from userspace is negative. IPv4's do_ip_getsockopt() already does
this.

> IPv4's do_ip_getsockopt() rejects a negative optlen right after reading
> it. do_ipv6_getsockopt() never has, and nothing downstream treats it as
> an error either: len is an int, but every consumer compares it unsigned,
> so -1 behaves as a huge value and each site clamps to its own reply
> size.

[Severity: Low]

Is this an accurate description of the old behaviour? Two cases in
do_ipv6_getsockopt() in net/ipv6/ipv6_sockglue.c don't seem to match it.

For MCAST_MSFILTER, ipv6_get_msfilter() and compat_ipv6_get_msfilter()
already use a signed compare, because size0 is a const int:

	const int size0 = offsetof(struct group_filter, gf_slist_flex);
	...
	if (len < size0)
		return -EINVAL;

So len = -1 already returned -EINVAL before this patch.

For IPV6_2292PKTOPTIONS on a SOCK_STREAM socket, len is never compared
or clamped:

		msg.msg_controllen = len;

msg_controllen is __kernel_size_t, so -1 becomes SIZE_MAX. That value
passes both size checks in put_cmsg():

  msg_controllen < sizeof(struct cmsghdr)
  msg_controllen < cmlen

As a result, every pending cmsg is written to optval with no limit from
the real buffer size. This covers the cmsgs from ip6_datagram_recv_ctl()
and from the sticky rxopt put_cmsg() calls. After that:

		len -= msg.msg_controllen;

reports the number of bytes written, not a clamped reply size.

The writes only land in the caller's own address space. The BPF kernel
sockptr path through sol_ipv6_sockopt() only allows IPV6_TCLASS,
IPV6_AUTOFLOWLABEL and IPV6_TRANSPARENT with *optlen == sizeof(int).
So this is a changelog problem only, and the code change covers both
paths.

Could the commit message mention the IPV6_2292PKTOPTIONS case, and drop
the claim that every consumer compares len unsigned?

> This is a bug ready to bite us in the near future, let's get this fixed.
>
> I've found this because testing the rest of the patch was returning
> inconsistency when optlen = -1.
>
> Later patches make the check unreachable, since sockopt_init_user() and
> sockptr_to_sockopt() both reject a negative length before the switch
> runs. Keep it as a guard; the fix stands on its own here.

[Severity: Low]

Is this meant as a standalone fix, or as preparatory hardening for the
series?

Userspace can see the change on the baseline. The IPv6 proto_ops have no
getsockopt_iter, so do_sock_getsockopt() takes the legacy
ops->getsockopt path and never reaches sockptr_to_sockopt():

do_sock_getsockopt()
  sock_common_getsockopt()
    udpv6_getsockopt()
      ipv6_getsockopt()
        do_ipv6_getsockopt()

Nothing on that path checks the sign of len. With len = -1,
getsockopt(udp6_fd, SOL_IPV6, IPV6_TCLASS, buf, &len) reaches:

	len = min_t(unsigned int, sizeof(int), len);

It then writes 4 bytes and returns 0. With this patch it returns -EINVAL.

If this is a fix, should it have a Fixes: tag and target net? The
unchecked len predates git history, so something like this would fit:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

If it is hardening for the net-next series, could "a bug ready to bite
us" and "the fix stands on its own here" be reworded?

Also, "the rest of the patch" and "Later patches make the check
unreachable" refer to the series. Once this is in git history, readers
won't have that context. Could those sentences be reworded too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-sockopt_expand_out_v2-v1-0-c3ef2e3bb5c0%40debian.org