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:03:36 +0300

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.

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