From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: ifconf(): return error if copyout() failed
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 11 Nov 2025 11:24:32 +0100

On Tue, Nov 11, 2025 at 01:01:20PM +0300, Vitaliy Makkoveev wrote:
> Currently if copyout() of interface address failed we continue with the
> next interface, override error value and finally we modify `ifc_len' of
> passed request. Return error value just after fail.
> 
> Also stop processing if userland buffer is full. No reason for lists
> iterations null-ops.

Yes, the old logic looks odd.  If copyout fails, do not try to
copyout seomthing else.

You missed
                                } else {
                                        space -= sa->sa_len - sizeof(*sa);
                                        if (space < sizeof (ifr))
                                                break;
This should be goto out.

Also we should not modify space before the length check.  If we
abort the loop, we do not report the actual copied length to userland.

bluhm

> Index: sys/net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> diff -u -p -r1.745 if.c
> --- sys/net/if.c	11 Nov 2025 07:56:50 -0000	1.745
> +++ sys/net/if.c	11 Nov 2025 09:52:01 -0000
> @@ -2734,7 +2734,7 @@ ifconf(caddr_t data)
>  	struct ifnet *ifp;
>  	struct ifaddr *ifa;
>  	struct ifreq ifr, *ifrp;
> -	int space = ifc->ifc_len, error = 0;
> +	int space = ifc->ifc_len, error;
>  
>  	/* If ifc->ifc_len is 0, fill it in with the needed size and return. */
>  	if (space == 0) {
> @@ -2768,20 +2768,22 @@ ifconf(caddr_t data)
>  			error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
>  			    sizeof(ifr));
>  			if (error)
> -				break;
> +				return (error);
>  			space -= sizeof (ifr), ifrp++;
>  		} else
>  			TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
>  				struct sockaddr *sa = ifa->ifa_addr;
>  
>  				if (space < sizeof(ifr))
> -					break;
> +					goto out;
>  				if (sa->sa_len <= sizeof(*sa)) {
>  					memset(&ifr.ifr_addr, 0,
>  					    sizeof(ifr.ifr_addr));
>  					memcpy(&ifr.ifr_addr, sa, sa->sa_len);
>  					error = copyout((caddr_t)&ifr,
>  					    (caddr_t)ifrp, sizeof (ifr));
> +					if (error)
> +						return (error);
>  					ifrp++;
>  				} else {
>  					space -= sa->sa_len - sizeof(*sa);
> @@ -2790,20 +2792,22 @@ ifconf(caddr_t data)
>  					error = copyout((caddr_t)&ifr,
>  					    (caddr_t)ifrp,
>  					    sizeof(ifr.ifr_name));
> -					if (error == 0)
> -						error = copyout((caddr_t)sa,
> -						    (caddr_t)&ifrp->ifr_addr,
> -						    sa->sa_len);
> +					if (error)
> +						return (error);
> +					error = copyout((caddr_t)sa,
> +					    (caddr_t)&ifrp->ifr_addr,
> +					    sa->sa_len);
> +					if (error)
> +						return (error);
>  					ifrp = (struct ifreq *)(sa->sa_len +
>  					    (caddr_t)&ifrp->ifr_addr);
>  				}
> -				if (error)
> -					break;
>  				space -= sizeof (ifr);
>  			}
>  	}
> +out:
>  	ifc->ifc_len -= space;
> -	return (error);
> +	return (0);
>  }
>  
>  void