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 13:33:14 +0100

On Tue, Nov 11, 2025 at 03:03:36PM +0300, Vitaliy Makkoveev wrote:
> On Tue, Nov 11, 2025 at 11:24:32AM +0100, Alexander Bluhm wrote:
> > 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.
> > 
> 
> Thanks, I missed that.
> 
> > 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
> > 
> 
> In the 'else' branch we copy (sizeof(ifr.ifr_name) + sa->sa_len) bytes
> of data so I prefer to do explicit calculation and check against this
> value.
> 
> Also I prefer to explicitly move `ifrp' pointer and decrease `space'
> within each branch separately.

much better

> +					ifrp = (void *)ifrp + total;

Is length arithmetic with void pointer allowed?  I think this should
be ifrp = (caddr_t)ifrp + total;

OK 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 11:49:35 -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,42 +2768,52 @@ ifconf(caddr_t data)
>  			error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
>  			    sizeof(ifr));
>  			if (error)
> -				break;
> -			space -= sizeof (ifr), ifrp++;
> +				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);
> +
> +					space -= sizeof(ifr);
>  					ifrp++;
>  				} else {
> -					space -= sa->sa_len - sizeof(*sa);
> -					if (space < sizeof (ifr))
> -						break;
> +					int total = sizeof(ifr.ifr_name) +
> +					    sa->sa_len;
> +
> +					if (space < total)
> +						goto out;
>  					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);
> -					ifrp = (struct ifreq *)(sa->sa_len +
> -					    (caddr_t)&ifrp->ifr_addr);
> +					if (error)
> +						return (error);
> +					error = copyout((caddr_t)sa,
> +					    (caddr_t)&ifrp->ifr_addr,
> +					    sa->sa_len);
> +					if (error)
> +						return (error);
> +
> +					space -= total;
> +					ifrp = (void *)ifrp + total;
>  				}
> -				if (error)
> -					break;
> -				space -= sizeof (ifr);
>  			}
>  	}
> +out:
>  	ifc->ifc_len -= space;
> -	return (error);
> +	return (0);
>  }
>  
>  void