On Tue, Apr 18, 2023 at 10:44:36PM +0000, Klemens Nanni wrote:
> 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?
Ping. Rebased onto the error cleanup.
Index: netinet/in.c
===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.183
diff -u -p -r1.183 in.c
--- netinet/in.c 21 Apr 2023 00:41:13 -0000 1.183
+++ netinet/in.c 23 Apr 2023 23:31:00 -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,9 +199,8 @@ 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;
- privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
privileged++;
@@ -235,10 +234,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:
@@ -247,6 +250,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)
@@ -275,11 +281,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) {
@@ -335,8 +336,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;
@@ -348,9 +348,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);
@@ -395,8 +392,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;
@@ -411,9 +407,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
if (error)
return (error);
}
-
- if (!privileged)
- return (EPERM);
KERNEL_LOCK();
NET_LOCK();
Index: netinet6/in6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.261
diff -u -p -r1.261 in6.c
--- netinet6/in6.c 21 Apr 2023 00:41:13 -0000 1.261
+++ netinet6/in6.c 23 Apr 2023 23:31:35 -0000
@@ -196,9 +196,8 @@ 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;
- privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
privileged++;
