Re: [PATCH net-next 4/6] ipv4: convert do_ip_getsockopt() to sockopt_t

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: MCAST_MSFILTER leaks modified optlen to userspace on copy failure
--

commit 28c83ba3c12df5eadd41490e34288c12faadfb4f
Author: Breno Leitao <leitao@debian.org>
Subject: ipv4: convert do_ip_getsockopt() to sockopt_t

This commit converts the SOL_IP switch and ip_mroute_getsockopt() to use
sockopt_t. It updates ip_getsockopt() to write optlen back unconditionally,
and modifies ip_mc_msfget() to store its length last so it doesn't report
an updated length if it fails.

> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
[ ... ]
> @@ -1783,19 +1759,22 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
>  int ip_getsockopt(struct sock *sk, int level,
>  		  int optname, char __user *optval, int __user *optlen)
>  {
> +	sockopt_t opt;
>  	int err;
>  
> -	err = do_ip_getsockopt(sk, level, optname,
> -			       USER_SOCKPTR(optval), USER_SOCKPTR(optlen));
> +	err = sockopt_init_user(&opt, optval, optlen);
> +	if (err)
> +		return err;
> +
> +	err = do_ip_getsockopt(sk, level, optname, &opt);
> +	if (put_user(opt.optlen, optlen))
> +		return -EFAULT;

[Severity: Medium]
Since put_user() now unconditionally writes opt.optlen back even if
do_ip_getsockopt() returns an error, does this leak modified lengths to
userspace on failure for MCAST_MSFILTER?

The commit message notes that ip_mc_msfget() was changed to store its length
last because of this unconditional writeback. However, the MCAST_MSFILTER
handlers (ip_get_mcast_msfilter() and compat_ip_get_mcast_msfilter()) still
set opt->optlen before their final copy_to_iter() calls.

For example, in ip_get_mcast_msfilter():

	opt->optlen = gsf_size;
	/* ip_mc_gsfget() consumed the whole reply; rewind to the fixed part. */
	iov_iter_revert(&opt->iter_out, gsf_size);
	if (copy_to_iter(&gsf, size0, &opt->iter_out) != size0)
		return -EFAULT;

If this final copy fails, do_ip_getsockopt() will return -EFAULT, but the
updated optlen will still be written to userspace.

A similar pattern exists in compat_ip_get_mcast_msfilter():

	opt->optlen = len;
	/* Rewind to gf_fmode, which gf_numsrc follows. */
	iov_iter_revert(&opt->iter_out,
			len - offsetof(struct compat_group_filter, gf_fmode));
	if (copy_to_iter(&gf.gf_fmode, sizeof(gf32.gf_fmode),
			 &opt->iter_out) != sizeof(gf32.gf_fmode) ||
	    copy_to_iter(&gf.gf_numsrc, sizeof(gf32.gf_numsrc),
			 &opt->iter_out) != sizeof(gf32.gf_numsrc))
		return -EFAULT;

Should these handlers also be updated to set opt->optlen only after all
copies have succeeded, matching the change made to ip_mc_msfget()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-sockopt_expand_out_v2-v1-0-c3ef2e3bb5c0@debian.org?part=4