On 31/05/16(Tue) 15:52, Gerhard Roth wrote:
> On Mon, 23 May 2016 17:47:28 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> > On 23/05/16(Mon) 16:51, Gerhard Roth wrote:
> > > On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot <m...@openbsd.org>
> > > wrote:
> > > > On 23/05/16(Mon) 15:38, Gerhard Roth wrote:
> > > >
> > > > This is crazy :) No driver should ever modify `ia' directly. This
> > > > code should call in_control() via the ioctl path.
> > >
> > > As mentioned in a previous mail: this was mostly copied from
> > > if_spppsubr.c:sppp_set_ip_addrs(). But doing an SIOCSIFADDR
> > > ioctl() from inside the kernel seems weird, too.
> >
> > SIOCAIFADDR/SIOCDIFADDR It is the way to go. The driver should not
> > manipulate addresses or route entry.
>
> Not manipulating the route entries is simple to fix. Will do that.
>
> But using SIOCAIFADDR/SIOCDIFADDR seems rather awkward since in_control()
> requires a 'struct socket *so' argument (even though it does nothing with
> it, except checking 'so->so_state & SS_PRIV'). Creating a socket inside
> the driver for this sole purpose seems just as weird as setting up a
> fake struct socket.
>
> Are you really sure, this is the better way to go?
I'm not sure it it is the better way to go. But I'm sure a driver
should not manipulate `struct ifa' or `struct rtentry'.
Now it should be fairly easy to split in_control() in two and pass a
``privileged'' variable based on the SS_PRIV flag. in6_control()
already does half of this ;)
I'd be interested to know if the diff below help, and if it does, does
it simplifies your actual code?
Index: netinet/in.c
===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.127
diff -u -p -r1.127 in.c
--- netinet/in.c 18 Apr 2016 06:43:51 -0000 1.127
+++ netinet/in.c 1 Jun 2016 07:50:29 -0000
@@ -84,8 +84,8 @@
void in_socktrim(struct sockaddr_in *);
void in_len2mask(struct in_addr *, int);
-int in_lifaddr_ioctl(struct socket *, u_long, caddr_t,
- struct ifnet *);
+int in_ioctl(u_long, caddr_t, struct ifnet *, int);
+int in_lifaddr_ioctl(u_long, caddr_t, struct ifnet *, int);
void in_purgeaddr(struct ifaddr *);
int in_addhost(struct in_ifaddr *, struct sockaddr_in *);
@@ -172,14 +172,11 @@ in_len2mask(struct in_addr *mask, int le
int
in_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
{
- struct ifreq *ifr = (struct ifreq *)data;
- struct ifaddr *ifa;
- struct in_ifaddr *ia = NULL;
- struct in_aliasreq *ifra = (struct in_aliasreq *)data;
- struct sockaddr_in oldaddr;
- int error;
- int newifaddr;
- int s;
+ int privileged;
+
+ privileged = 0;
+ if ((so->so_state & SS_PRIV) != 0)
+ privileged++;
switch (cmd) {
#ifdef MROUTING
@@ -189,18 +186,33 @@ in_control(struct socket *so, u_long cmd
#endif /* MROUTING */
case SIOCALIFADDR:
case SIOCDLIFADDR:
- if ((so->so_state & SS_PRIV) == 0)
+ if (!privileged)
return (EPERM);
/* FALLTHROUGH */
case SIOCGLIFADDR:
if (ifp == NULL)
return (EINVAL);
- return in_lifaddr_ioctl(so, cmd, data, ifp);
+ return in_lifaddr_ioctl(cmd, data, ifp, privileged);
default:
if (ifp == NULL)
return (EOPNOTSUPP);
}
+ return (in_ioctl(cmd, data, ifp, privileged));
+}
+
+int
+in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
+{
+ struct ifreq *ifr = (struct ifreq *)data;
+ struct ifaddr *ifa;
+ struct in_ifaddr *ia = NULL;
+ struct in_aliasreq *ifra = (struct in_aliasreq *)data;
+ struct sockaddr_in oldaddr;
+ int error;
+ int newifaddr;
+ int s;
+
TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
if (ifa->ifa_addr->sa_family == AF_INET) {
ia = ifatoia(ifa);
@@ -225,7 +237,7 @@ in_control(struct socket *so, u_long cmd
return (EADDRNOTAVAIL);
/* FALLTHROUGH */
case SIOCSIFADDR:
- if ((so->so_state & SS_PRIV) == 0)
+ if (!privileged)
return (EPERM);
if (ia == NULL) {
@@ -250,7 +262,7 @@ in_control(struct socket *so, u_long cmd
case SIOCSIFNETMASK:
case SIOCSIFDSTADDR:
case SIOCSIFBRDADDR:
- if ((so->so_state & SS_PRIV) == 0)
+ if (!privileged)
return (EPERM);
/* FALLTHROUGH */
@@ -410,8 +422,7 @@ in_control(struct socket *so, u_long cmd
* other values may be returned from in_ioctl()
*/
int
-in_lifaddr_ioctl(struct socket *so, u_long cmd, caddr_t data,
- struct ifnet *ifp)
+in_lifaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
{
struct if_laddrreq *iflr = (struct if_laddrreq *)data;
struct ifaddr *ifa;
@@ -481,7 +492,7 @@ in_lifaddr_ioctl(struct socket *so, u_lo
ifra.ifra_mask.sin_len = sizeof(struct sockaddr_in);
in_len2mask(&ifra.ifra_mask.sin_addr, iflr->prefixlen);
- return in_control(so, SIOCAIFADDR, (caddr_t)&ifra, ifp);
+ return in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, privileged);
}
case SIOCGLIFADDR:
case SIOCDLIFADDR:
@@ -566,7 +577,8 @@ in_lifaddr_ioctl(struct socket *so, u_lo
memcpy(&ifra.ifra_dstaddr, &ia->ia_sockmask,
ia->ia_sockmask.sin_len);
- return in_control(so, SIOCDIFADDR, (caddr_t)&ifra, ifp);
+ return in_ioctl(SIOCDIFADDR, (caddr_t)&ifra, ifp,
+ privileged);
}
}
}
Index: netinet6/in6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.186
diff -u -p -r1.186 in6.c
--- netinet6/in6.c 3 Mar 2016 12:57:15 -0000 1.186
+++ netinet6/in6.c 1 Jun 2016 07:54:20 -0000
@@ -118,7 +118,8 @@ const struct in6_addr in6mask64 = IN6MAS
const struct in6_addr in6mask96 = IN6MASK96;
const struct in6_addr in6mask128 = IN6MASK128;
-int in6_lifaddr_ioctl(struct socket *, u_long, caddr_t, struct ifnet *);
+int in6_lifaddr_ioctl(u_long, caddr_t, struct ifnet *, int);
+int in6_ioctl(u_long, caddr_t, struct ifnet *, int);
int in6_ifinit(struct ifnet *, struct in6_ifaddr *, int);
void in6_unlink_ifa(struct in6_ifaddr *, struct ifnet *);
@@ -165,11 +166,7 @@ in6_mask2len(struct in6_addr *mask, u_ch
int
in6_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
{
- struct in6_ifreq *ifr = (struct in6_ifreq *)data;
- struct in6_ifaddr *ia6 = NULL;
- struct in6_aliasreq *ifra = (struct in6_aliasreq *)data;
- struct sockaddr_in6 *sa6;
- int s, privileged;
+ int privileged;
privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
@@ -183,6 +180,18 @@ in6_control(struct socket *so, u_long cm
}
#endif
+ return (in6_ioctl(cmd, data, ifp, privileged));
+}
+
+int
+in6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
+{
+ struct in6_ifreq *ifr = (struct in6_ifreq *)data;
+ struct in6_ifaddr *ia6 = NULL;
+ struct in6_aliasreq *ifra = (struct in6_aliasreq *)data;
+ struct sockaddr_in6 *sa6;
+ int s;
+
if (ifp == NULL)
return (EOPNOTSUPP);
@@ -206,7 +215,7 @@ in6_control(struct socket *so, u_long cm
return (EPERM);
/* FALLTHROUGH */
case SIOCGLIFADDR:
- return in6_lifaddr_ioctl(so, cmd, data, ifp);
+ return in6_lifaddr_ioctl(cmd, data, ifp, privileged);
}
/*
@@ -939,8 +948,7 @@ in6_unlink_ifa(struct in6_ifaddr *ia6, s
* address encoding scheme. (see figure on page 8)
*/
int
-in6_lifaddr_ioctl(struct socket *so, u_long cmd, caddr_t data,
- struct ifnet *ifp)
+in6_lifaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
{
struct if_laddrreq *iflr = (struct if_laddrreq *)data;
struct ifaddr *ifa;
@@ -1047,7 +1055,8 @@ in6_lifaddr_ioctl(struct socket *so, u_l
in6_prefixlen2mask(&ifra.ifra_prefixmask.sin6_addr, prefixlen);
ifra.ifra_flags = iflr->flags & ~IFLR_PREFIX;
- return in6_control(so, SIOCAIFADDR_IN6, (caddr_t)&ifra, ifp);
+ return in6_ioctl(SIOCAIFADDR_IN6, (caddr_t)&ifra, ifp,
+ privileged);
}
case SIOCGLIFADDR:
case SIOCDLIFADDR:
@@ -1142,8 +1151,8 @@ in6_lifaddr_ioctl(struct socket *so, u_l
ia6->ia_prefixmask.sin6_len);
ifra.ifra_flags = ia6->ia6_flags;
- return in6_control(so, SIOCDIFADDR_IN6, (caddr_t)&ifra,
- ifp);
+ return in6_ioctl(SIOCDIFADDR_IN6, (caddr_t)&ifra, ifp,
+ privileged);
}
}
}
