Re: explicitly check broadcast addresses on some ifa_ifwithaddr() uses
On 12/02/15 20:06, Martin Pieuchot wrote:
> On 02/12/15(Wed) 16:18, Vincent Gross wrote:
>> When fed a broadcast address, ifa_ifwitaddr() returns the unicast ifa
>> whose broadcast address match the input. This is used mainly to select
>> ifa, and there can be trouble when you have 2 ifas on the same range
>> (e.g. 10.0.0.1/24@em0 & 10.0.0.20/24@em1) :
>>
>> netinet/ip_mroute.c:814
>> net/route.c:785
>> netinet/ip_divert.c:143
>> net/if_vxlan.c:241
>>
>> There are also places where broadcast addresses should not be tolerated :
>>
>> netinet/ip_input.c:1061 broadcast address is not a module identifier
>> netinet/ip_input.c:1141 see above
>> netinet/ip_input.c:1197 see above
>> netinet6/*: no broadcast in ipv6
>> net/route.c:562: gateway shall never be a broadcast addr
>> net/route.c:713: see above
>>
>> This diff removes broadcast matching from ifa_ifwithaddr, and
>> adds or rewrites checks where necessary.
>>
>> Comments ? Ok ?
>
> Looks good to me. Some nits below.
Nits applied.
Anyone else ?
Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.417
diff -u -p -r1.417 if.c
--- sys/net/if.c2 Dec 2015 16:35:52 - 1.417
+++ sys/net/if.c3 Dec 2015 07:59:53 -
@@ -1179,13 +1179,6 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
if (equal(addr, ifa->ifa_addr))
return (ifa);
-
- /* IPv6 doesn't have broadcast */
- if ((ifp->if_flags & IFF_BROADCAST) &&
- ifa->ifa_broadaddr &&
- ifa->ifa_broadaddr->sa_len != 0 &&
- equal(ifa->ifa_broadaddr, addr))
- return (ifa);
}
}
return (NULL);
Index: sys/netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.190
diff -u -p -r1.190 in_pcb.c
--- sys/netinet/in_pcb.c2 Dec 2015 22:13:44 - 1.190
+++ sys/netinet/in_pcb.c3 Dec 2015 07:59:53 -
@@ -332,14 +332,13 @@ in_pcbbind(struct inpcb *inp, struct mbu
ia = ifatoia(ifa_ifwithaddr(sintosa(sin),
inp->inp_rtableid));
- if (ia == NULL)
- return (EADDRNOTAVAIL);
/* SOCK_RAW does not use in_pcbbind() */
- if (so->so_type != SOCK_DGRAM &&
- sin->sin_addr.s_addr !=
- ia->ia_addr.sin_addr.s_addr)
- return (EADDRNOTAVAIL);
+ if (ia == NULL &&
+ (so->so_type != SOCK_DGRAM ||
+ !in_broadcast(sin->sin_addr,
+ inp->inp_rtableid)))
+ return (EADDRNOTAVAIL);
}
}
if (lport) {
@@ -353,7 +352,8 @@ in_pcbbind(struct inpcb *inp, struct mbu
t = in_pcblookup(table, _addr, 0,
>sin_addr, lport, INPLOOKUP_WILDCARD,
inp->inp_rtableid);
- if (t && (so->so_euid !=
t->inp_socket->so_euid))
+ if (t &&
+ (so->so_euid != t->inp_socket->so_euid))
return (EADDRINUSE);
}
t = in_pcblookup(table, _addr, 0,
Index: sys/netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.311
diff -u -p -r1.311 ip_output.c
--- sys/netinet/ip_output.c 2 Dec 2015 20:50:20 - 1.311
+++ sys/netinet/ip_output.c 3 Dec 2015 07:59:53 -
@@ -1368,13 +1368,12 @@ ip_setmoptions(int optname, struct ip_mo
sin.sin_family = AF_INET;
sin.sin_addr = addr;
ia = ifatoia(ifa_ifwithaddr(sintosa(), rtableid));
- if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
- ifp = ia->ia_ifp;
- if (ifp == NULL || (ifp->if_flags & IFF_MULTICAST) == 0) {
+ if (ia == NULL ||
+ (ia->ia_ifp->if_flags & IFF_MULTICAST) == 0) {
error = EADDRNOTAVAIL;
break;
}
- imo->imo_ifidx = ifp->if_index;
+ imo->imo_ifidx = ia->ia_ifp->if_index;
break;
case IP_MULTICAST_TTL:
@@ -1542,12 +1541,11 @@ ip_setmoptions(int optname, struct ip_mo
Re: explicitly check broadcast addresses on some ifa_ifwithaddr() uses
On 12/03/15 10:21, Vincent Gross wrote:
> On 12/02/15 20:06, Martin Pieuchot wrote:
>> On 02/12/15(Wed) 16:18, Vincent Gross wrote:
>>> When fed a broadcast address, ifa_ifwitaddr() returns the unicast ifa
>>> whose broadcast address match the input. This is used mainly to select
>>> ifa, and there can be trouble when you have 2 ifas on the same range
>>> (e.g. 10.0.0.1/24@em0 & 10.0.0.20/24@em1) :
>>>
>>> netinet/ip_mroute.c:814
>>> net/route.c:785
>>> netinet/ip_divert.c:143
>>> net/if_vxlan.c:241
>>>
>>> There are also places where broadcast addresses should not be tolerated :
>>>
>>> netinet/ip_input.c:1061 broadcast address is not a module identifier
>>> netinet/ip_input.c:1141 see above
>>> netinet/ip_input.c:1197 see above
>>> netinet6/*: no broadcast in ipv6
>>> net/route.c:562: gateway shall never be a broadcast addr
>>> net/route.c:713: see above
>>>
>>> This diff removes broadcast matching from ifa_ifwithaddr, and
>>> adds or rewrites checks where necessary.
>>>
>>> Comments ? Ok ?
>>
>> Looks good to me. Some nits below.
>
> Nits applied.
>
> Anyone else ?
bluhm@ spotted one case where in_broadcast was needed.
ok ?
Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.418
diff -u -p -r1.418 if.c
--- sys/net/if.c3 Dec 2015 12:22:51 - 1.418
+++ sys/net/if.c3 Dec 2015 13:48:58 -
@@ -1220,13 +1220,6 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
if (equal(addr, ifa->ifa_addr))
return (ifa);
-
- /* IPv6 doesn't have broadcast */
- if ((ifp->if_flags & IFF_BROADCAST) &&
- ifa->ifa_broadaddr &&
- ifa->ifa_broadaddr->sa_len != 0 &&
- equal(ifa->ifa_broadaddr, addr))
- return (ifa);
}
}
return (NULL);
Index: sys/net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.283
diff -u -p -r1.283 route.c
--- sys/net/route.c 2 Dec 2015 16:49:58 - 1.283
+++ sys/net/route.c 3 Dec 2015 13:49:00 -
@@ -539,7 +539,9 @@ rtredirect(struct sockaddr *dst, struct
bcmp((caddr_t)(a1), (caddr_t)(a2), (a1)->sa_len) == 0)
if (rt != NULL && (!equal(src, rt->rt_gateway) || rt->rt_ifa != ifa))
error = EINVAL;
- else if (ifa_ifwithaddr(gateway, rdomain) != NULL)
+ else if (ifa_ifwithaddr(gateway, rdomain) != NULL ||
+ (gateway->sa_family = AF_INET &&
+ in_broadcast(satosin(gateway)->sin_addr, rdomain)))
error = EHOSTUNREACH;
if (error)
goto done;
Index: sys/netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.191
diff -u -p -r1.191 in_pcb.c
--- sys/netinet/in_pcb.c3 Dec 2015 09:49:15 - 1.191
+++ sys/netinet/in_pcb.c3 Dec 2015 13:49:00 -
@@ -332,14 +332,13 @@ in_pcbbind(struct inpcb *inp, struct mbu
ia = ifatoia(ifa_ifwithaddr(sintosa(sin),
inp->inp_rtableid));
- if (ia == NULL)
- return (EADDRNOTAVAIL);
/* SOCK_RAW does not use in_pcbbind() */
- if (so->so_type != SOCK_DGRAM &&
- sin->sin_addr.s_addr !=
- ia->ia_addr.sin_addr.s_addr)
- return (EADDRNOTAVAIL);
+ if (ia == NULL &&
+ (so->so_type != SOCK_DGRAM ||
+ !in_broadcast(sin->sin_addr,
+ inp->inp_rtableid)))
+ return (EADDRNOTAVAIL);
}
}
if (lport) {
@@ -353,7 +352,8 @@ in_pcbbind(struct inpcb *inp, struct mbu
t = in_pcblookup(table, _addr, 0,
>sin_addr, lport, INPLOOKUP_WILDCARD,
inp->inp_rtableid);
- if (t && (so->so_euid !=
t->inp_socket->so_euid))
+ if (t &&
+ (so->so_euid != t->inp_socket->so_euid))
return (EADDRINUSE);
}
t = in_pcblookup(table, _addr, 0,
Index: sys/netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.311
diff -u -p
Re: explicitly check broadcast addresses on some ifa_ifwithaddr() uses
On Thu, Dec 03, 2015 at 02:52:59PM +0100, Vincent Gross wrote:
> bluhm@ spotted one case where in_broadcast was needed.
>
> ok ?
OK bluhm@
>
> Index: sys/net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.418
> diff -u -p -r1.418 if.c
> --- sys/net/if.c 3 Dec 2015 12:22:51 - 1.418
> +++ sys/net/if.c 3 Dec 2015 13:48:58 -
> @@ -1220,13 +1220,6 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
>
> if (equal(addr, ifa->ifa_addr))
> return (ifa);
> -
> - /* IPv6 doesn't have broadcast */
> - if ((ifp->if_flags & IFF_BROADCAST) &&
> - ifa->ifa_broadaddr &&
> - ifa->ifa_broadaddr->sa_len != 0 &&
> - equal(ifa->ifa_broadaddr, addr))
> - return (ifa);
> }
> }
> return (NULL);
> Index: sys/net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.283
> diff -u -p -r1.283 route.c
> --- sys/net/route.c 2 Dec 2015 16:49:58 - 1.283
> +++ sys/net/route.c 3 Dec 2015 13:49:00 -
> @@ -539,7 +539,9 @@ rtredirect(struct sockaddr *dst, struct
>bcmp((caddr_t)(a1), (caddr_t)(a2), (a1)->sa_len) == 0)
> if (rt != NULL && (!equal(src, rt->rt_gateway) || rt->rt_ifa != ifa))
> error = EINVAL;
> - else if (ifa_ifwithaddr(gateway, rdomain) != NULL)
> + else if (ifa_ifwithaddr(gateway, rdomain) != NULL ||
> + (gateway->sa_family = AF_INET &&
> + in_broadcast(satosin(gateway)->sin_addr, rdomain)))
> error = EHOSTUNREACH;
> if (error)
> goto done;
> Index: sys/netinet/in_pcb.c
> ===
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.191
> diff -u -p -r1.191 in_pcb.c
> --- sys/netinet/in_pcb.c 3 Dec 2015 09:49:15 - 1.191
> +++ sys/netinet/in_pcb.c 3 Dec 2015 13:49:00 -
> @@ -332,14 +332,13 @@ in_pcbbind(struct inpcb *inp, struct mbu
>
> ia = ifatoia(ifa_ifwithaddr(sintosa(sin),
> inp->inp_rtableid));
> - if (ia == NULL)
> - return (EADDRNOTAVAIL);
>
> /* SOCK_RAW does not use in_pcbbind() */
> - if (so->so_type != SOCK_DGRAM &&
> - sin->sin_addr.s_addr !=
> - ia->ia_addr.sin_addr.s_addr)
> - return (EADDRNOTAVAIL);
> + if (ia == NULL &&
> + (so->so_type != SOCK_DGRAM ||
> + !in_broadcast(sin->sin_addr,
> + inp->inp_rtableid)))
> + return (EADDRNOTAVAIL);
> }
> }
> if (lport) {
> @@ -353,7 +352,8 @@ in_pcbbind(struct inpcb *inp, struct mbu
> t = in_pcblookup(table, _addr, 0,
> >sin_addr, lport, INPLOOKUP_WILDCARD,
> inp->inp_rtableid);
> - if (t && (so->so_euid !=
> t->inp_socket->so_euid))
> + if (t &&
> + (so->so_euid != t->inp_socket->so_euid))
> return (EADDRINUSE);
> }
> t = in_pcblookup(table, _addr, 0,
> Index: sys/netinet/ip_output.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.311
> diff -u -p -r1.311 ip_output.c
> --- sys/netinet/ip_output.c 2 Dec 2015 20:50:20 - 1.311
> +++ sys/netinet/ip_output.c 3 Dec 2015 13:49:01 -
> @@ -1368,13 +1368,12 @@ ip_setmoptions(int optname, struct ip_mo
> sin.sin_family = AF_INET;
> sin.sin_addr = addr;
> ia = ifatoia(ifa_ifwithaddr(sintosa(), rtableid));
> - if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
> - ifp = ia->ia_ifp;
> - if (ifp == NULL || (ifp->if_flags & IFF_MULTICAST) == 0) {
> + if (ia == NULL ||
> + (ia->ia_ifp->if_flags & IFF_MULTICAST) == 0) {
> error = EADDRNOTAVAIL;
> break;
> }
> - imo->imo_ifidx = ifp->if_index;
> + imo->imo_ifidx = ia->ia_ifp->if_index;
> break;
>
> case IP_MULTICAST_TTL:
> @@ -1542,12 +1541,11 @@ ip_setmoptions(int optname, struct ip_mo
>
explicitly check broadcast addresses on some ifa_ifwithaddr() uses
When fed a broadcast address, ifa_ifwitaddr() returns the unicast ifa
whose broadcast address match the input. This is used mainly to select
ifa, and there can be trouble when you have 2 ifas on the same range
(e.g. 10.0.0.1/24@em0 & 10.0.0.20/24@em1) :
netinet/ip_mroute.c:814
net/route.c:785
netinet/ip_divert.c:143
net/if_vxlan.c:241
There are also places where broadcast addresses should not be tolerated :
netinet/ip_input.c:1061 broadcast address is not a module identifier
netinet/ip_input.c:1141 see above
netinet/ip_input.c:1197 see above
netinet6/*: no broadcast in ipv6
net/route.c:562: gateway shall never be a broadcast addr
net/route.c:713: see above
This diff removes broadcast matching from ifa_ifwithaddr, and
adds or rewrites checks where necessary.
Comments ? Ok ?
Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.416
diff -u -p -r1.416 if.c
--- sys/net/if.c2 Dec 2015 08:47:00 - 1.416
+++ sys/net/if.c2 Dec 2015 15:17:26 -
@@ -1178,13 +1178,6 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
if (equal(addr, ifa->ifa_addr))
return (ifa);
-
- /* IPv6 doesn't have broadcast */
- if ((ifp->if_flags & IFF_BROADCAST) &&
- ifa->ifa_broadaddr &&
- ifa->ifa_broadaddr->sa_len != 0 &&
- equal(ifa->ifa_broadaddr, addr))
- return (ifa);
}
}
return (NULL);
Index: sys/netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.188
diff -u -p -r1.188 in_pcb.c
--- sys/netinet/in_pcb.c30 Oct 2015 09:39:42 - 1.188
+++ sys/netinet/in_pcb.c2 Dec 2015 15:17:26 -
@@ -328,14 +328,12 @@ in_pcbbind(struct inpcb *inp, struct mbu
ia = ifatoia(ifa_ifwithaddr(sintosa(sin),
inp->inp_rtableid));
- if (ia == NULL)
- return (EADDRNOTAVAIL);
/* SOCK_RAW does not use in_pcbbind() */
- if (so->so_type != SOCK_DGRAM &&
- sin->sin_addr.s_addr !=
- ia->ia_addr.sin_addr.s_addr)
- return (EADDRNOTAVAIL);
+ if (ia == NULL &&
+ (so->so_type != SOCK_DGRAM ||
+ !in_broadcast(sin->sin_addr,
inp->inp_rtableid)))
+ return (EADDRNOTAVAIL);
}
}
if (lport) {
Index: sys/netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.310
diff -u -p -r1.310 ip_output.c
--- sys/netinet/ip_output.c 2 Dec 2015 13:29:26 - 1.310
+++ sys/netinet/ip_output.c 2 Dec 2015 15:17:27 -
@@ -1387,9 +1387,8 @@ ip_setmoptions(int optname, struct ip_mo
sin.sin_family = AF_INET;
sin.sin_addr = addr;
ia = ifatoia(ifa_ifwithaddr(sintosa(), rtableid));
- if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
- ifp = ia->ia_ifp;
- if (ifp == NULL || (ifp->if_flags & IFF_MULTICAST) == 0) {
+ if (ia == NULL || (ifp = ia->ia_ifp) == NULL ||
+ (ia->ia_ifp->if_flags & IFF_MULTICAST) == 0) {
error = EADDRNOTAVAIL;
break;
}
@@ -1561,12 +1560,11 @@ ip_setmoptions(int optname, struct ip_mo
sin.sin_family = AF_INET;
sin.sin_addr = mreq->imr_interface;
ia = ifatoia(ifa_ifwithaddr(sintosa(), rtableid));
- if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
- ifp = ia->ia_ifp;
- else {
+ if (ia == NULL) {
error = EADDRNOTAVAIL;
break;
}
+ ifp = ia->ia_ifp;
}
/*
* Find the membership in the membership array.
Index: sys/netinet/raw_ip.c
===
RCS file: /cvs/src/sys/netinet/raw_ip.c,v
retrieving revision 1.84
diff -u -p -r1.84 raw_ip.c
--- sys/netinet/raw_ip.c28 Jul 2015 12:22:07 - 1.84
+++ sys/netinet/raw_ip.c2 Dec 2015 15:17:27 -
@@ -473,6 +473,7 @@
Re: explicitly check broadcast addresses on some ifa_ifwithaddr() uses
On 02/12/15(Wed) 16:18, Vincent Gross wrote:
> When fed a broadcast address, ifa_ifwitaddr() returns the unicast ifa
> whose broadcast address match the input. This is used mainly to select
> ifa, and there can be trouble when you have 2 ifas on the same range
> (e.g. 10.0.0.1/24@em0 & 10.0.0.20/24@em1) :
>
> netinet/ip_mroute.c:814
> net/route.c:785
> netinet/ip_divert.c:143
> net/if_vxlan.c:241
>
> There are also places where broadcast addresses should not be tolerated :
>
> netinet/ip_input.c:1061 broadcast address is not a module identifier
> netinet/ip_input.c:1141 see above
> netinet/ip_input.c:1197 see above
> netinet6/*: no broadcast in ipv6
> net/route.c:562: gateway shall never be a broadcast addr
> net/route.c:713: see above
>
> This diff removes broadcast matching from ifa_ifwithaddr, and
> adds or rewrites checks where necessary.
>
> Comments ? Ok ?
Looks good to me. Some nits below.
> Index: sys/netinet/in_pcb.c
> ===
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.188
> diff -u -p -r1.188 in_pcb.c
> --- sys/netinet/in_pcb.c 30 Oct 2015 09:39:42 - 1.188
> +++ sys/netinet/in_pcb.c 2 Dec 2015 15:17:26 -
> @@ -328,14 +328,12 @@ in_pcbbind(struct inpcb *inp, struct mbu
>
> ia = ifatoia(ifa_ifwithaddr(sintosa(sin),
> inp->inp_rtableid));
> - if (ia == NULL)
> - return (EADDRNOTAVAIL);
>
> /* SOCK_RAW does not use in_pcbbind() */
> - if (so->so_type != SOCK_DGRAM &&
> - sin->sin_addr.s_addr !=
> - ia->ia_addr.sin_addr.s_addr)
> - return (EADDRNOTAVAIL);
> + if (ia == NULL &&
> + (so->so_type != SOCK_DGRAM ||
> + !in_broadcast(sin->sin_addr, inp->inp_rtable
id)))
^
Code should fit in 80 columns.
> + return (EADDRNOTAVAIL);
> }
> }
> if (lport) {
> Index: sys/netinet/ip_output.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.310
> diff -u -p -r1.310 ip_output.c
> --- sys/netinet/ip_output.c 2 Dec 2015 13:29:26 - 1.310
> +++ sys/netinet/ip_output.c 2 Dec 2015 15:17:27 -
> @@ -1387,9 +1387,8 @@ ip_setmoptions(int optname, struct ip_mo
> sin.sin_family = AF_INET;
> sin.sin_addr = addr;
> ia = ifatoia(ifa_ifwithaddr(sintosa(), rtableid));
> - if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
> - ifp = ia->ia_ifp;
> - if (ifp == NULL || (ifp->if_flags & IFF_MULTICAST) == 0) {
> + if (ia == NULL || (ifp = ia->ia_ifp) == NULL ||
ia_ifp MUST not be NULL since you got it from the per-ifp list, so need
to check for NULL.
> + (ia->ia_ifp->if_flags & IFF_MULTICAST) == 0) {
> error = EADDRNOTAVAIL;
> break;
> }
