Re: in_ioctl*: hoist identical privilege checks
On Sun, Apr 23, 2023 at 11:33:57PM +, Klemens Nanni wrote:
> > > 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 would surprise me if some userland would rely on the error code
when the ioctl is called unpriviledged. The only way to find out,
is to commit this diff.
> @@ -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++;
>
This is just style change. I don't know which style is better.
Please do not change it.
> @@ -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++;
>
You do not even touch this function. Such changes make digging
history harder.
otherwise OK bluhm@
Re: in_ioctl*: hoist identical privilege checks
On Tue, Apr 18, 2023 at 10:44:36PM +, Klemens Nanni wrote:
> On Sat, Apr 15, 2023 at 01:48:02PM +, Klemens Nanni wrote:
> > On Fri, Apr 14, 2023 at 11:33:18PM +, 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.c21 Apr 2023 00:41:13 - 1.183
+++ netinet/in.c23 Apr 2023 23:31:00 -
@@ -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 - 1.261
+++ netinet6/in6.c 23 Apr 2023 23:31:35 -
@@ -196,9 +196,8 @@ in6_sa2sin6(struct sockaddr *sa, struct
int
in6_control(struct socket *s
Re: in_ioctl*: hoist identical privilege checks
On Sat, Apr 15, 2023 at 01:48:02PM +, Klemens Nanni wrote:
> On Fri, Apr 14, 2023 at 11:33:18PM +, 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 - 1.259
+++ netinet6/in6.c 18 Apr 2023 22:40:40 -
@@ -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.c18 Apr 2023 22:20:16 - 1.181
+++ netinet/in.c18 Apr 2023 22:40:41 -
@@ -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 (!p
Re: in_ioctl*: hoist identical privilege checks
On Fri, Apr 14, 2023 at 11:33:18PM +, 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.c15 Apr 2023 13:24:47 - 1.180
+++ netinet/in.c15 Apr 2023 13:45:56 -
@@ -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) {
in_ioctl*: hoist identical privilege checks
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.
Feedback? Objection OK?
Index: in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.179
diff -u -p -r1.179 in.c
--- in.c6 Dec 2022 22:19:39 - 1.179
+++ in.c14 Apr 2023 23:05:30 -
@@ -283,13 +283,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;
@@ -309,11 +309,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;
@@ -325,11 +320,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;
@@ -427,6 +417,9 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
return (error);
}
+ if (!privileged)
+ return (EPERM);
+
NET_LOCK();
TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
@@ -444,11 +437,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;
@@ -531,11 +519,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
break;
}
case SIOCDIFADDR:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if (ia == NULL) {
error = EADDRNOTAVAIL;
break;
