On Sat, Apr 15, 2023 at 01:48:02PM +0000, Klemens Nanni wrote:
> 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.
Committed.
> > 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.
Better and rebased diff, no locks taken for unprivileged calls that *will*
fail, EPERM is returned first in IPv4 like IPv6 already does, nicely syncing
the ioctl handlers.
Feedback? OK?
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 18 Apr 2023 22:40:40 -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++;
Index: netinet/in.c
===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.181
diff -u -p -r1.181 in.c
--- netinet/in.c 18 Apr 2023 22:20:16 -0000 1.181
+++ netinet/in.c 18 Apr 2023 22:40:41 -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:
@@ -254,6 +257,9 @@ in_ioctl(u_long cmd, caddr_t data, struc
return (EOPNOTSUPP);
}
+ if (!privileged)
+ return (EPERM);
+
if (ifr->ifr_addr.sa_family == AF_INET) {
error = in_sa2sin(&ifr->ifr_addr, &sin);
if (error)
@@ -282,11 +288,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
goto err;
}
- if (!privileged) {
- error = EPERM;
- goto err;
- }
-
switch (cmd) {
case SIOCSIFDSTADDR:
if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
@@ -342,8 +343,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;
@@ -355,9 +355,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);
@@ -402,8 +399,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;
@@ -418,9 +414,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
if (error)
return (error);
}
-
- if (!privileged)
- return (EPERM);
KERNEL_LOCK();
NET_LOCK();
