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.
One thing that I don't particularly like is that the diff introduces
some code duplication for allocating and initializing the ia in
SIOCSIFADDR and SIOCAIFADDR. I tried a few things to avoid it in this
diff, but it always resulted in tangled code paths, so a bit counter to
the purpose of the whole exercise. I intend to revisit this point later.
Index: sys/netinet/in.c
===================================================================
RCS file: /var/cvs/src/sys/netinet/in.c,v
retrieving revision 1.153
diff -u -p -r1.153 in.c
--- sys/netinet/in.c 28 May 2018 10:46:46 -0000 1.153
+++ sys/netinet/in.c 29 May 2018 04:55:24 -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);
}
@@ -329,27 +333,21 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
}
}
- switch (cmd) {
- case SIOCAIFADDR:
- case SIOCDIFADDR:
- if (ifra->ifra_addr.sin_family == AF_INET) {
- 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 ==
- ifra->ifra_addr.sin_addr.s_addr)
- break;
- }
- ia = ifatoia(ifa);
- }
- if (cmd == SIOCDIFADDR && ia == NULL) {
- error = EADDRNOTAVAIL;
- goto err;
+ if (ifra->ifra_addr.sin_family == AF_INET) {
+ 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 ==
+ ifra->ifra_addr.sin_addr.s_addr)
+ break;
}
- /* FALLTHROUGH */
+ ia = ifatoia(ifa);
+ }
+
+ switch (cmd) {
case SIOCSIFADDR:
if (!privileged) {
error = EPERM;
- goto err;
+ break;
}
if (ia == NULL) {
@@ -369,10 +367,7 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
newifaddr = 1;
} else
newifaddr = 0;
- break;
- }
- switch(cmd) {
- case SIOCSIFADDR:
+
in_ifscrub(ifp, ia);
error = in_ifinit(ifp, ia, satosin(&ifr->ifr_addr), newifaddr);
if (error)
@@ -383,6 +378,29 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
case SIOCAIFADDR: {
int needinit = 0;
+ if (!privileged) {
+ error = EPERM;
+ break;
+ }
+
+ if (ia == NULL) {
+ ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
+ ia->ia_addr.sin_family = AF_INET;
+ ia->ia_addr.sin_len = sizeof(ia->ia_addr);
+ ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr);
+ ia->ia_ifa.ifa_dstaddr = sintosa(&ia->ia_dstaddr);
+ ia->ia_ifa.ifa_netmask = sintosa(&ia->ia_sockmask);
+ ia->ia_sockmask.sin_len = 8;
+ if (ifp->if_flags & IFF_BROADCAST) {
+ ia->ia_broadaddr.sin_len = sizeof(ia->ia_addr);
+ ia->ia_broadaddr.sin_family = AF_INET;
+ }
+ ia->ia_ifp = ifp;
+
+ newifaddr = 1;
+ } else
+ newifaddr = 0;
+
if (ia->ia_addr.sin_family == AF_INET) {
if (ifra->ifra_addr.sin_len == 0)
ifra->ifra_addr = ia->ia_addr;
@@ -419,6 +437,15 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
break;
}
case SIOCDIFADDR:
+ if (!privileged) {
+ error = EPERM;
+ break;
+ }
+
+ if (ia == NULL) {
+ error = EADDRNOTAVAIL;
+ break;
+ }
/*
* Even if the individual steps were safe, shouldn't
* these kinds of changes happen atomically? What
@@ -433,7 +460,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
panic("invalid ioctl %lu", cmd);
}
-err:
NET_UNLOCK();
return (error);
}
