On 15/10/15(Thu) 14:09, Vincent Gross wrote:
> 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 ?
ok mpi@
>
>
> 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);
>
