On Fri, Apr 14, 2023 at 11:33:18PM +0000, Klemens Nanni wrote:
> All cases do the same check up first, so merge it before the switch.
>
> It could be hoisted further in both in_ioctl() and in_ioctl_change_ifaddr(),
> but that meant a change in errno return semantic, so leave it for now.
in6.c already has the privilege check as early as possible, so here's a diff
that simplifies in.c to match in6.c.
I can't think of a scenario where returning EPERM (this diff) instead of
whatever errno the currently earlier sanity checks yield would break.
It is an unprivileged ioctl call in the first place, so EPERM as soon as
possible makes sense to me.
This nicely drops 23 lines and a needless argument in two functions,
matching in6_ioctl*().
Feedback? Objection? OK?
Index: netinet/in.c
===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.180
diff -u -p -r1.180 in.c
--- netinet/in.c 15 Apr 2023 13:24:47 -0000 1.180
+++ netinet/in.c 15 Apr 2023 13:45:56 -0000
@@ -84,8 +84,8 @@
void in_socktrim(struct sockaddr_in *);
-int in_ioctl_set_ifaddr(u_long, caddr_t, struct ifnet *, int);
-int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *, int);
+int in_ioctl_set_ifaddr(u_long, caddr_t, struct ifnet *);
+int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *);
int in_ioctl_get(u_long, caddr_t, struct ifnet *);
void in_purgeaddr(struct ifaddr *);
int in_addhost(struct in_ifaddr *, struct sockaddr_in *);
@@ -199,10 +199,9 @@ in_sa2sin(struct sockaddr *sa, struct so
int
in_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
{
- int privileged;
+ int privileged = 0;
int error;
- privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
privileged++;
@@ -242,10 +241,14 @@ in_ioctl(u_long cmd, caddr_t data, struc
case SIOCGIFBRDADDR:
return in_ioctl_get(cmd, data, ifp);
case SIOCSIFADDR:
- return in_ioctl_set_ifaddr(cmd, data, ifp, privileged);
+ if (!privileged)
+ return (EPERM);
+ return in_ioctl_set_ifaddr(cmd, data, ifp);
case SIOCAIFADDR:
case SIOCDIFADDR:
- return in_ioctl_change_ifaddr(cmd, data, ifp, privileged);
+ if (!privileged)
+ return (EPERM);
+ return in_ioctl_change_ifaddr(cmd, data, ifp);
case SIOCSIFNETMASK:
case SIOCSIFDSTADDR:
case SIOCSIFBRDADDR:
@@ -282,13 +285,13 @@ in_ioctl(u_long cmd, caddr_t data, struc
goto err;
}
+ if (!privileged) {
+ error = EPERM;
+ goto err;
+ }
+
switch (cmd) {
case SIOCSIFDSTADDR:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
error = EINVAL;
break;
@@ -308,11 +311,6 @@ 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;
@@ -324,11 +322,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
break;
case SIOCSIFNETMASK:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if (ifr->ifr_addr.sa_len < 8) {
error = EINVAL;
break;
@@ -352,8 +345,7 @@ err:
}
int
-in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
- int privileged)
+in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp)
{
struct ifreq *ifr = (struct ifreq *)data;
struct ifaddr *ifa;
@@ -365,9 +357,6 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t
if (cmd != SIOCSIFADDR)
panic("%s: invalid ioctl %lu", __func__, cmd);
- if (!privileged)
- return (EPERM);
-
error = in_sa2sin(&ifr->ifr_addr, &sin);
if (error)
return (error);
@@ -412,8 +401,7 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t
}
int
-in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
- int privileged)
+in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp)
{
struct ifaddr *ifa;
struct in_ifaddr *ia = NULL;
@@ -447,11 +435,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
case SIOCAIFADDR: {
int needinit = 0;
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if (ifra->ifra_mask.sin_len) {
if (ifra->ifra_mask.sin_len < 8) {
error = EINVAL;
@@ -534,11 +517,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
break;
}
case SIOCDIFADDR:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if (ia == NULL) {
error = EADDRNOTAVAIL;
break;
Index: netinet6/in6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.259
diff -u -p -r1.259 in6.c
--- netinet6/in6.c 6 Dec 2022 22:19:39 -0000 1.259
+++ netinet6/in6.c 15 Apr 2023 13:45:57 -0000
@@ -196,10 +196,9 @@ in6_sa2sin6(struct sockaddr *sa, struct
int
in6_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
{
- int privileged;
+ int privileged = 0;
int error;
- privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
privileged++;
