Re: [PATCH net-next 2/2] ipv4: igmp: convert ip_mc_msfget() to sockopt_t

[netdev-bot+sashiko]

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

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

- [Medium] In do_ip_getsockopt() (net/ipv4/ip_sockglue.c, case
  IP_MSFILTER), the output length is now written back to userspace…

--

ipv4: igmp: convert ip_mc_msfget() to sockopt_t

This converts ip_mc_msfget() from the sockptr_t optval/optlen pair to a
sockopt_t built at the do_ip_getsockopt() call site, and replaces the
optval copies with sequential copy_to_iter() calls after growing the
output iterator with sockopt_expand_out().

> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index d56355aca7977..144fca158adcb 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c

[ ... ]

> @@ -2755,14 +2755,19 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
>  	len = flex_array_size(psl, sl_addr, copycount);
>  	msf->imsf_numsrc = count;
>  	msf_size = IP_MSFILTER_SIZE(copycount);
> -	if (copy_to_sockptr(optlen, &msf_size, sizeof(int)) ||
> -	    copy_to_sockptr(optval, msf, IP_MSFILTER_SIZE(0))) {
> +
> +	/* The source list is sized by the imsf_numsrc the caller left in
> +	 * optval, not by optlen, which only has to cover the fixed part.
> +	 */
> +	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) !=
> +	    IP_MSFILTER_SIZE(0))
>  		return -EFAULT;
> -	}
> -	if (len &&
> -	    copy_to_sockptr_offset(optval,
> -				   offsetof(struct ip_msfilter, imsf_slist_flex),
> -				   psl->sl_addr, len))
> +	if (len && copy_to_iter(psl->sl_addr, len, &opt->iter_out) != len)
>  		return -EFAULT;
>  	return 0;
>  done:

> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index a55ef327ec932..c58e565f2a5aa 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c

[ ... ]

> @@ -1715,7 +1717,13 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
>  			err = -EFAULT;
>  			goto out;
>  		}
> -		err = ip_mc_msfget(sk, &msf, optval, optlen);
> +		err = sockptr_to_sockopt(&opt, optval, optlen, &kvec);
> +		if (err)
> +			goto out;
> +
> +		err = ip_mc_msfget(sk, &msf, &opt);
> +		if (copy_to_sockptr(optlen, &opt.optlen, sizeof(int)))
> +			err = -EFAULT;
>  		goto out;

[Severity: Medium]
Should this write-back be guarded by if (!err)?

Before this patch the *optlen store lived inside ip_mc_msfget() and only
ran once the lookup had succeeded:

	msf_size = IP_MSFILTER_SIZE(copycount);
	if (copy_to_sockptr(optlen, &msf_size, sizeof(int)) ||
	    copy_to_sockptr(optval, msf, IP_MSFILTER_SIZE(0))) {
		return -EFAULT;
	}

All of the ip_mc_msfget() failure returns left the user's optlen word
untouched: !ipv4_is_multicast(addr) returning -EINVAL, !in_dev returning
-ENODEV, and the !pmc "must have a prior join" case falling through to
done: with err = -EADDRNOTAVAIL.

Now do_ip_getsockopt() writes opt.optlen back on every return from
ip_mc_msfget(), including those three error paths and the new -EINVAL
from sockopt_expand_out(), since opt->optlen is only assigned on the
success path in ip_mc_msfget(). Is that change of behaviour on failure
intended?

There is a second effect from the missing guard: if the optlen pointer is
readable but not writable, copy_to_sockptr() fails and err is replaced
with -EFAULT, so -EINVAL, -ENODEV and -EADDRNOTAVAIL are no longer
distinguishable by userspace. The earlier copy_from_sockptr(&len, optlen,
...) and the copy_from_sockptr() inside sockptr_to_sockopt() both succeed
on a read-only mapping, so this state is reachable.

Would something like this preserve the old semantics?

	err = ip_mc_msfget(sk, &msf, &opt);
	if (!err && copy_to_sockptr(optlen, &opt.optlen, sizeof(int)))
		err = -EFAULT;

The commit message describes the change as a conversion, with "the bytes
land where they did", and does not mention that the optlen store moved to
the caller and now also runs when ip_mc_msfget() fails. Could that be
called out explicitly?

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