On 10/07/15 14:05, Martin Pieuchot wrote:
> On 01/10/15(Thu) 19:40, Vincent Gross wrote:
>> Although the sysctls controlling the port range are labelled
>> "port(hi)?first" and
>> "port(hi)?last", no ordering is enforced and you can have portfirst >
>> portlast.
>> in_pcbbind() (and in6_pcbsetport()) work around this by duplicating the loop
>> looking
>> for an available port.
>
> What about a small regress test?
>
>> This diff introduce temporary bounds and compare them to guarantee that
>> first <= last, thus allowing deduplication of the port scan loop.
>
> Makes sense to me, I'd keep the comment though.
>
>>
>> Tested on my laptop with a narrow port range and heavy cheezburger browsing,
>> no fault
>> detected. Deeper testing welcome.
>>
>> Should I include in6_pcbsetport() changes right now or should ipv4 be
>> validated first ?
>
> I prefer when both version are keep in sync, so yes a in6_pcbsetport()
> diff would be nice. Plus if it's possible to have a regress test it
> would be awesome.
>
Ok ?
Index: sys/netinet/in_pcb.c
===================================================================
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.181
diff -u -p -r1.181 in_pcb.c
--- sys/netinet/in_pcb.c 9 Oct 2015 01:10:27 -0000 1.181
+++ sys/netinet/in_pcb.c 15 Oct 2015 12:08:06 -0000
@@ -360,67 +360,47 @@ in_pcbbind(struct inpcb *inp, struct mbu
inp->inp_laddr = sin->sin_addr;
}
if (lport == 0) {
- u_int16_t first, last;
+ u_int16_t bound_a, bound_b, first, last;
int count;
if (inp->inp_flags & INP_HIGHPORT) {
- first = ipport_hifirstauto; /* sysctl */
- last = ipport_hilastauto;
+ bound_a = ipport_hifirstauto; /* sysctl */
+ bound_b = ipport_hilastauto;
} else if (inp->inp_flags & INP_LOWPORT) {
if ((error = suser(p, 0)))
return (EACCES);
- first = IPPORT_RESERVED-1; /* 1023 */
- last = 600; /* not IPPORT_RESERVED/2 */
+ bound_a = IPPORT_RESERVED-1; /* 1023 */
+ bound_b = 600; /* not IPPORT_RESERVED/2 */
} else {
- first = ipport_firstauto; /* sysctl */
- last = ipport_lastauto;
+ bound_a = ipport_firstauto; /* sysctl */
+ bound_b = ipport_lastauto;
+ }
+ if (bound_a < bound_b) {
+ first = bound_a;
+ last = bound_b;
+ } else {
+ first = bound_b;
+ last = bound_a;
}
/*
* Simple check to ensure all ports are not used up causing
* a deadlock here.
- *
- * We split the two cases (up and down) so that the direction
- * is not being tested on each round of the loop.
*/
- if (first > last) {
- /*
- * counting down
- */
- count = first - last;
- if (count)
- lastport = first - arc4random_uniform(count);
+ count = last - first;
+ lastport = first + arc4random_uniform(count);
- do {
- if (count-- < 0) /* completely used? */
- return (EADDRNOTAVAIL);
- --lastport;
- if (lastport > first || lastport < last)
- lastport = first;
- lport = htons(lastport);
- } while (in_baddynamic(lastport,
so->so_proto->pr_protocol) ||
- in_pcblookup(table, &zeroin_addr, 0,
- &inp->inp_laddr, lport, wild, inp->inp_rtableid));
- } else {
- /*
- * counting up
- */
- count = last - first;
- if (count)
- lastport = first + arc4random_uniform(count);
-
- do {
- if (count-- < 0) /* completely used? */
- return (EADDRNOTAVAIL);
- ++lastport;
- if (lastport < first || lastport > last)
- lastport = first;
- lport = htons(lastport);
- } while (in_baddynamic(lastport,
so->so_proto->pr_protocol) ||
- in_pcblookup(table, &zeroin_addr, 0,
- &inp->inp_laddr, lport, wild, inp->inp_rtableid));
- }
+ do {
+ if (count-- < 0) /* completely used? */
+ return (EADDRNOTAVAIL);
+ ++lastport;
+ if (lastport < first || lastport > last)
+ lastport = first;
+ lport = htons(lastport);
+ } while (in_baddynamic(lastport, so->so_proto->pr_protocol) ||
+ in_pcblookup(table, &zeroin_addr, 0,
+ &inp->inp_laddr, lport, wild, inp->inp_rtableid));
}
inp->inp_lport = lport;
in_pcbrehash(inp);
Index: sys/netinet6/in6_pcb.c
===================================================================
RCS file: /cvs/src/sys/netinet6/in6_pcb.c,v
retrieving revision 1.77
diff -u -p -r1.77 in6_pcb.c
--- sys/netinet6/in6_pcb.c 15 Oct 2015 10:27:18 -0000 1.77
+++ sys/netinet6/in6_pcb.c 15 Oct 2015 12:08:06 -0000
@@ -293,7 +293,7 @@ in6_pcbsetport(struct in6_addr *laddr, s
{
struct socket *so = inp->inp_socket;
struct inpcbtable *table = inp->inp_table;
- u_int16_t first, last;
+ u_int16_t bound_a, bound_b, first, last;
u_int16_t lastport = 0;
u_int16_t lport = 0;
int count;
@@ -308,63 +308,43 @@ in6_pcbsetport(struct in6_addr *laddr, s
wild |= INPLOOKUP_WILDCARD;
if (inp->inp_flags & INP_HIGHPORT) {
- first = ipport_hifirstauto; /* sysctl */
- last = ipport_hilastauto;
+ bound_a = ipport_hifirstauto; /* sysctl */
+ bound_b = ipport_hilastauto;
} else if (inp->inp_flags & INP_LOWPORT) {
if ((error = suser(p, 0)))
return (EACCES);
- first = IPPORT_RESERVED-1; /* 1023 */
- last = 600; /* not IPPORT_RESERVED/2 */
+ bound_a = IPPORT_RESERVED-1; /* 1023 */
+ bound_b = 600; /* not IPPORT_RESERVED/2 */
} else {
- first = ipport_firstauto; /* sysctl */
- last = ipport_lastauto;
+ bound_a = ipport_firstauto; /* sysctl */
+ bound_b = ipport_lastauto;
+ }
+ if (bound_a < bound_b) {
+ first = bound_a;
+ last = bound_b;
+ } else {
+ first = bound_b;
+ last = bound_a;
}
/*
* Simple check to ensure all ports are not used up causing
* a deadlock here.
- *
- * We split the two cases (up and down) so that the direction
- * is not being tested on each round of the loop.
*/
- if (first > last) {
- /*
- * counting down
- */
- count = first - last;
- if (count)
- lastport = first - arc4random_uniform(count);
-
- do {
- if (count-- < 0) /* completely used? */
- return (EADDRNOTAVAIL);
- --lastport;
- if (lastport > first || lastport < last)
- lastport = first;
- lport = htons(lastport);
- } while (in_baddynamic(lastport, so->so_proto->pr_protocol) ||
- in_pcblookup(table, &zeroin6_addr, 0,
- &inp->inp_laddr6, lport, wild, inp->inp_rtableid));
- } else {
- /*
- * counting up
- */
- count = last - first;
- if (count)
- lastport = first + arc4random_uniform(count);
-
- do {
- if (count-- < 0) /* completely used? */
- return (EADDRNOTAVAIL);
- ++lastport;
- if (lastport < first || lastport > last)
- lastport = first;
- lport = htons(lastport);
- } while (in_baddynamic(lastport, so->so_proto->pr_protocol) ||
- in_pcblookup(table, &zeroin6_addr, 0,
- &inp->inp_laddr6, lport, wild, inp->inp_rtableid));
- }
+ count = last - first;
+ lastport = first + arc4random_uniform(count);
+
+ do {
+ if (count-- < 0) /* completely used? */
+ return (EADDRNOTAVAIL);
+ ++lastport;
+ if (lastport < first || lastport > last)
+ lastport = first;
+ lport = htons(lastport);
+ } while (in_baddynamic(lastport, so->so_proto->pr_protocol) ||
+ in_pcblookup(table, &zeroin6_addr, 0,
+ &inp->inp_laddr6, lport, wild, inp->inp_rtableid));
inp->inp_lport = lport;
in_pcbrehash(inp);
