netdev - Re: [PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191124160955.3cf26f53@cakuba.netronome.com>
Date:   Sun, 24 Nov 2019 16:09:55 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Maciej Żenczykowski <zenczykowski@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Linux NetDev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH] net: Fix a documentation bug wrt.
 ip_unprivileged_port_start

On Sun, 24 Nov 2019 01:16:35 -0800, Maciej Żenczykowski wrote:
> > Since this is a documentation _bug_ :) we probably need a Fixes tag.
> > The mistake is almost 3 years old, could be worth giving the backport
> > bots^W folks a chance to pick it up.
> >
> > Is this all the way from 4548b683b781 ("Introduce a sysctl that
> > modifies the value of PROT_SOCK.") ?  
> 
> Yes, indeed.
> That commit adds the documentation itself, and:
> 
> // ipv4_local_port_range()
> -               if (range[1] < range[0])
> +               /* Ensure that the upper limit is not smaller than the lower,
> +                * and that the lower does not encroach upon the privileged
> +                * port limit.
> +                */
> +               if ((range[1] < range[0]) ||
> +                   (range[0] < net->ipv4.sysctl_ip_prot_sock))
> 
> and
> 
> // ipv4_privileged_ports()
> 
> +       pports = net->ipv4.sysctl_ip_prot_sock;
> +
> +       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> +
> +       if (write && ret == 0) {
> +               inet_get_local_port_range(net, &range[0], &range[1]);
> +               /* Ensure that the local port range doesn't overlap with the
> +                * privileged port range.
> +                */
> +               if (range[0] < pports)
> +                       ret = -EINVAL;
> +               else
> +                       net->ipv4.sysctl_ip_prot_sock = pports;
> +       }
> 
> Anyway, I guess this means this commit should have:
> 
> Fixes: 4548b683b781 ("Introduce a sysctl that modifies the value of PROT_SOCK.")
> 
> (which is in v4.10-rc4-733-g4548b683b781)
> 
> Should I resubmit with the new tag, or will you just pick it up?

Thanks for the tag, now that you showed me the code I'm not 100% sure
the doc is correct :S

The first unprivileged port has to be lower than the
ip_local_port_range, IOW ip_local_port_range must fall 
into unprivileged range entirely.

So does "It may not overlap with the ip_local_port_range." really
express that? Should it say something like "ip_local_port_range must
fall into the unprivileged range" or "It must be lower or equal to start
of ip_local_port_range" or "Unprivileged port range must contain
ip_local_port_range" or...