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

On Tue, Nov 11, 2025 at 01:33:14PM +0100, Alexander Bluhm wrote:
> 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;
> 

It is allowed with both llvm and gcc unless the option -Wpointer-arith
was used. However, the (caddr_t) cast is better. I'll commit with
caddr_t.

> 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
>