Re: in_ioctl: one case per ioctl

[Theo Buehler]

On Wed, May 30, 2018 at 04:49:15AM +0200, Theo Buehler wrote:
> We can finally get rid of one switch in both, in_ioctl() and
> in_ioctl_change_ifaddr(). With this diff we have one case per ioctl,
> each case dealing with an ioctl starts with a privilege check before any
> global data is modified and the code paths are now straightforward.

Unfortunately, I saw too late that the first version of this diff
contained a mistake in the SIOCSIFADDR case. Let's go more slowly and
let me do this in two steps instead.

Here's the unchanged first part of the diff dealing with in_ioctl()
again. I'll send a fixed version of the second part once this is in.

Index: sys/netinet/in.c
===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.155
diff -u -p -r1.155 in.c
--- sys/netinet/in.c    31 May 2018 11:49:26 -0000      1.155
+++ sys/netinet/in.c    31 May 2018 12:00:57 -0000
@@ -248,33 +248,28 @@ in_ioctl(u_long cmd, caddr_t data, struc
                }
        }
 
+       if (ia && satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
+               for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) {
+                       if ((ifa->ifa_addr->sa_family == AF_INET) &&
+                           ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+                           satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
+                               ia = ifatoia(ifa);
+                               break;
+                       }
+               }
+       }
+       if (ia == NULL) {
+               NET_UNLOCK();
+               return (EADDRNOTAVAIL);
+       }
+
        switch (cmd) {
-       case SIOCSIFNETMASK:
        case SIOCSIFDSTADDR:
-       case SIOCSIFBRDADDR:
                if (!privileged) {
                        error = EPERM;
-                       goto err;
+                       break;
                }
 
-               if (ia && satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
-                       for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) {
-                               if ((ifa->ifa_addr->sa_family == AF_INET) &&
-                                   ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
-                                   satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
-                                       ia = ifatoia(ifa);
-                                       break;
-                               }
-                       }
-               }
-               if (ia == NULL) {
-                       error = EADDRNOTAVAIL;
-                       goto err;
-               }
-               break;
-       }
-       switch (cmd) {
-       case SIOCSIFDSTADDR:
                if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
                        error = EINVAL;
                        break;
@@ -291,6 +286,11 @@ in_ioctl(u_long cmd, caddr_t data, struc
                break;
 
        case SIOCSIFBRDADDR:
+               if (!privileged) {
+                       error = EPERM;
+                       break;
+               }
+
                if ((ifp->if_flags & IFF_BROADCAST) == 0) {
                        error = EINVAL;
                        break;
@@ -299,12 +299,16 @@ in_ioctl(u_long cmd, caddr_t data, struc
                break;
 
        case SIOCSIFNETMASK:
+               if (!privileged) {
+                       error = EPERM;
+                       break;
+               }
+
                ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr =
                    ifra->ifra_addr.sin_addr.s_addr;
                break;
        }
 
-err:
        NET_UNLOCK();
        return (error);
 }