Re: [PATCH net 1/2] ipv4: mcast: getsockopt: do not overwrite past optlen

On Thu, 06 Aug 2026 02:42:00 -0700
Breno Leitao <[email protected]> wrote:

> getsockopt(MCAST_MSFILTER) can write past the end of the buffer the caller
> declared.
> 
> This is because the copy_to_user() does not respect the optlen, and can
> write over the allocated buffer, overwriting userspace undesired
> memory

Nak. This is just the way it is defined.
The application provides a length that is just the header.
As you noted the header contains details of the real buffer.

There are quite a few sockopt like it, you have to support them.
There may be some where the length isn't checked and is just assumed
to be the right size - they have to continue to work as well.

	David

> 
> The amount written comes from the numsrc the caller left in optval, not
> from optlen. do_ip_getsockopt() reads optlen once, to check that the header
> fits, and then reuses the variable for the length of the reply, so by the
> time ip_mc_gsfget() fills the source list nothing remembers how big the
> buffer was.
> 
> The copies go through copy_to_user(), so this reaches only the caller's own
> address space.
> 
> setsockopt has had the matching check from the start:
> 
> 	if (GROUP_FILTER_SIZE(gsf->gf_numsrc) > optlen)
> 		return -EINVAL;
> 
> Clamp numsrc to what optlen holds rather than rejecting. Another option
> would be to reject (-EINVAL), but, that might break userspace _more_.
> 
> For reviewing purposes: size0 is the header size, so, the available
> buffer is len - size0.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Breno Leitao <[email protected]>
> ---
>  net/ipv4/ip_sockglue.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index a55ef327ec932..2e4e19b90645b 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -1447,6 +1447,7 @@ static int ip_get_mcast_msfilter(struct sock *sk, sockptr_t optval,
>  {
>  	const int size0 = offsetof(struct group_filter, gf_slist_flex);
>  	struct group_filter gsf;
> +	unsigned int max_numsrc;
>  	int num, gsf_size;
>  	int err;
>  
> @@ -1455,6 +1456,10 @@ static int ip_get_mcast_msfilter(struct sock *sk, sockptr_t optval,
>  	if (copy_from_sockptr(&gsf, optval, size0))
>  		return -EFAULT;
>  
> +	/* Maximum number of sources that would fit in the userspace buffer*/
> +	max_numsrc = (len - size0) / sizeof(gsf.gf_slist_flex[0]);
> +	gsf.gf_numsrc = min_t(u32, gsf.gf_numsrc, max_numsrc);
> +
>  	num = gsf.gf_numsrc;
>  	err = ip_mc_gsfget(sk, &gsf, optval,
>  			   offsetof(struct group_filter, gf_slist_flex));
> @@ -1474,6 +1479,7 @@ static int compat_ip_get_mcast_msfilter(struct sock *sk, sockptr_t optval,
>  {
>  	const int size0 = offsetof(struct compat_group_filter, gf_slist_flex);
>  	struct compat_group_filter gf32;
> +	unsigned int max_numsrc;
>  	struct group_filter gf;
>  	int num;
>  	int err;
> @@ -1483,6 +1489,9 @@ static int compat_ip_get_mcast_msfilter(struct sock *sk, sockptr_t optval,
>  	if (copy_from_sockptr(&gf32, optval, size0))
>  		return -EFAULT;
>  
> +	max_numsrc = (len - size0) / sizeof(gf32.gf_slist_flex[0]);
> +	gf32.gf_numsrc = min_t(u32, gf32.gf_numsrc, max_numsrc);
> +
>  	gf.gf_interface = gf32.gf_interface;
>  	gf.gf_fmode = gf32.gf_fmode;
>  	num = gf.gf_numsrc = gf32.gf_numsrc;
> @@ -1705,6 +1714,7 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
>  	switch (optname) {
>  	case IP_MSFILTER:
>  	{
> +		unsigned int max_numsrc;
>  		struct ip_msfilter msf;
>  
>  		if (len < IP_MSFILTER_SIZE(0)) {
> @@ -1715,6 +1725,12 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
>  			err = -EFAULT;
>  			goto out;
>  		}
> +		/* Do not write more sources than the caller said optval can
> +		 * hold.
> +		 */
> +		max_numsrc = (len - IP_MSFILTER_SIZE(0)) /
> +			     sizeof(msf.imsf_slist_flex[0]);
> +		msf.imsf_numsrc = min_t(u32, msf.imsf_numsrc, max_numsrc);
>  		err = ip_mc_msfget(sk, &msf, optval, optlen);
>  		goto out;
>  	}
>