Re: [PATCH net-next 1/2] net: add sockopt_expand_out()

[netdev-bot+sashiko]

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

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

- [Medium] sockopt_expand_out() (include/linux/net.h:82-93) mixes two
  length semantics: the growth predicate compares @size against…
- [Low] Undocumented linkage change: net/socket.c drops `static` from
  sockptr_to_sockopt() and include/linux/net.h publishes its prototype,…
- [Low] sockopt_expand_out() (include/linux/net.h:82-93) rejects a
  kernel-backed optval with WARN_ON_ONCE(!iter_is_ubuf(&opt->iter_out))…
- [Low] sockopt_expand_out() is introduced with no callers and the commit
  message never names the options that 'write past optlen today', so its…
- [Low] sockopt_expand_out() installs a caller-supplied size_t as the new
  count of a user-buffer iterator with no ceiling at all — no INT_MAX…

--

net: add sockopt_expand_out()

This adds a sockopt_expand_out() helper that regrows opt->iter_out past the
optlen userspace declared, for the getsockopt options whose reply length is
derived from a count stored in optval instead of from optlen.  It also makes
sockptr_to_sockopt() non-static and declares it in include/linux/net.h.

A few questions below.

> diff --git a/include/linux/net.h b/include/linux/net.h
> index 470100ae71077..de0ed362b3779 100644
> --- a/include/linux/net.h
> +++ b/include/linux/net.h
> @@ -70,6 +70,31 @@ static inline int sockopt_init_user(sockopt_t *opt, char __user *optval,
>  	return 0;
>  }
>  
> +/*
> + * Grow optval to @size, for the options whose reply is sized by a count the
> + * caller left in optval rather than by optlen. Those write past optlen today
> + * and userspace relies on it.
> + *
> + * Call it before writing through opt->iter_out: it re-anchors the iterator at
> + * the head of optval. Only a user buffer can be longer than the optlen the
> + * caller declared, so a kernel-backed optval is refused with -EINVAL.
> + */
> +static inline int sockopt_expand_out(sockopt_t *opt, size_t size)
> +{

[Severity: Low]
This isn't a bug, but the helper lands with no callers and the changelog
never names the options whose "reply is sized by a count the caller left in
optval".

The first user shows up in the following patch of the series, where
ip_mc_msfget() in net/ipv4/igmp.c calls it for the IP_MSFILTER reply.

Could the changelog name IP_MSFILTER, so the semantics of the new helper can
be read against a concrete conversion?

> +	if (size <= iov_iter_count(&opt->iter_out))
> +		return 0;
> +

[Severity: Medium]
This test compares @size against iov_iter_count(&opt->iter_out), which is the
number of bytes still remaining in the iterator, while the growth action below
re-anchors at the head of optval:

	iov_iter_ubuf(&opt->iter_out, ITER_DEST, opt->iter_out.ubuf, size);

For ITER_UBUF, iov_iter_advance() only bumps iov_offset and shrinks count;
ubuf keeps pointing at the head of the buffer.

So if a callback ever calls sockopt_expand_out() after copying part of its
reply, does the write cursor get rewound and the already-copied bytes
overwritten?

The same mismatch has two more effects: a @size that fits within the declared
optlen can still fall into the kernel-backed -EINVAL branch once the iterator
has been advanced, and the no-op branch can return 0 with the cursor sitting
mid-buffer.

opt->iter_in and opt->optlen also keep describing the old, smaller buffer
after the expansion.  Only ip_mc_msfget() assigning opt->optlen = msf_size
itself covers that up:

	err = sockopt_expand_out(opt, msf_size);
	if (err)
		return err;

	opt->optlen = msf_size;
	if (copy_to_iter(msf, IP_MSFILTER_SIZE(0), &opt->iter_out) !=

Nothing is broken today, since that single caller invokes the helper as the
first operation on a freshly built iterator, and the "call it before writing
through opt->iter_out" rule is stated in the comment.

Would comparing against opt->optlen (or iov_offset + count) and refusing a
partially consumed iterator make the helper's verdict independent of how much
has already been written?

> +	if (WARN_ON_ONCE(!iter_is_ubuf(&opt->iter_out)))
> +		return -EINVAL;
> +

[Severity: Low]
The changelog says only that "a kernel-backed optval keeps the bounded
iterator and the callback gets -EINVAL if it asks to grow", but this path also
emits a splat.

sockptr_to_sockopt() builds an ITER_KVEC iter_out whenever optval.is_kernel:

	if (optval.is_kernel) {
		kvec->iov_base = optval.kernel;
		kvec->iov_len = koptlen;
		iov_iter_kvec(&opt->iter_out, ITER_DEST, kvec, 1, koptlen);

so the state this WARN treats as impossible is a supported construction.

Following the reachability at the end of the series, the only caller of
sockopt_expand_out() is ip_mc_msfget(), reached only through the IP_MSFILTER
case of do_ip_getsockopt(), and the only kernel-backed entry into
do_ip_getsockopt() is sol_ip_sockopt() in net/core/filter.c:

	switch (optname) {
	case IP_TOS:
	case IP_TRANSPARENT:
		if (*optlen != sizeof(int))
			return -EINVAL;
		break;
	default:
		return -EINVAL;
	}

so IP_MSFILTER is rejected before any sockopt_t is built and no in-tree path
can trip the WARN.

Could the changelog mention the WARN as well, given it advertises only the
-EINVAL?

> +	iov_iter_ubuf(&opt->iter_out, ITER_DEST, opt->iter_out.ubuf, size);
> +
> +	return 0;
> +}
> +

[Severity: Low]
The caller-supplied @size becomes the new count of a user-buffer iterator with
no ceiling here: no INT_MAX clamp, even though opt->optlen and the getsockopt
optlen ABI are int, and no MAX_RW_COUNT limit of the kind import_ubuf()
applies to the same ITER_UBUF construction.  The comment above says @size
comes from "a count the caller left in optval", i.e. from user data.

At the only call site the value is bounded, so nothing overflows today:

	copycount = count < msf->imsf_numsrc ? count : msf->imsf_numsrc;
	len = flex_array_size(psl, sl_addr, copycount);
	msf->imsf_numsrc = count;
	msf_size = IP_MSFILTER_SIZE(copycount);

with psl->sl_count capped by net.ipv4.sysctl_igmp_max_msf, and the write past
the declared optlen matches what IP_MSFILTER already did before the
conversion.

Would a clamp on @size here, or a note that the helper is meant only for the
legacy options, be worth adding for future consumers?

> +int sockptr_to_sockopt(sockopt_t *opt, sockptr_t optval, sockptr_t optlen,
> +		       struct kvec *kvec);
> +
>  struct poll_table_struct;
>  struct pipe_inode_info;
>  struct inode;
> diff --git a/net/socket.c b/net/socket.c
> index c05d86e63abf7..29a0f7f8e2cab 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2437,8 +2437,8 @@ INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level,
>   * It is important to remember that both iov points to the same data, but,
>   * .iter_in is read-only and .iter_out is write-only by the protocol callbacks
>   */
> -static int sockptr_to_sockopt(sockopt_t *opt, sockptr_t optval,
> -			      sockptr_t optlen, struct kvec *kvec)
> +int sockptr_to_sockopt(sockopt_t *opt, sockptr_t optval,
> +		       sockptr_t optlen, struct kvec *kvec)

[Severity: Low]
The subject and changelog cover only sockopt_expand_out(), but this hunk also
drops static from sockptr_to_sockopt() and the header hunk publishes its
prototype.

The cross-file user only appears in the next patch, where do_ip_getsockopt()
in net/ipv4/ip_sockglue.c calls it:

	err = sockptr_to_sockopt(&opt, optval, optlen, &kvec);

Since ip_sockglue.o is obj-y and CONFIG_INET is a bool, no EXPORT_SYMBOL is
needed and nothing breaks.  Could the changelog say why sockptr_to_sockopt()
is being made global here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910-getsockopt_phase6-v1-0-e681e102d5b8%40debian.org