netdev - Re: sctp: bad hash index calculation

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] [day] [month] [year] [list]

Date:	Mon, 15 Feb 2016 10:59:51 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	linux-sctp@...r.kernel.org,
	Vladislav Yasevich <vyasevich@...il.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>
Subject: Re: sctp: bad hash index calculation

On Mon, Feb 15, 2016 at 04:56:01PM +0100, Dmitry Vyukov wrote:
> On Mon, Feb 15, 2016 at 4:50 PM, Neil Horman <nhorman@...driver.com> wrote:
> > On Mon, Feb 15, 2016 at 04:11:22PM +0100, Dmitry Vyukov wrote:
> >> Hello,
> >>
> >> While looking into some memory leaks of sctp ports I've noticed that
> >> sctp_init initializes port hash table as follows:
> >>
> >>     /* Allocate and initialize the SCTP port hash table.  */
> >>     do {
> >>         sctp_port_hashsize = (1UL << order) * PAGE_SIZE /
> >>                     sizeof(struct sctp_bind_hashbucket);
> >>         if ((sctp_port_hashsize > (64 * 1024)) && order > 0)
> >>             continue;
> >>         sctp_port_hashtable = (struct sctp_bind_hashbucket *)
> >>             __get_free_pages(GFP_KERNEL | __GFP_NOWARN, order);
> >>     } while (!sctp_port_hashtable && --order > 0);
> >>
> >> and then hash index is computed as follows:
> >>
> >> /* Warning: The following hash functions assume a power of two 'size'. */
> >> /* This is the hash function for the SCTP port hash table. */
> >> static inline int sctp_phashfn(struct net *net, __u16 lport)
> >> {
> >>     return (net_hash_mix(net) + lport) & (sctp_port_hashsize - 1);
> >> }
> >>
> >> I don't see what ensures that sctp_port_hashsize is in fact a power-of-2.
> >>
> >> spinlock_t in sctp_bind_hashbucket can be 2 words in some configs,
> >> then sizeof(sctp_bind_hashbucket) == 24, which can render half of hash
> >> table unused.
> >>
> >> struct sctp_bind_hashbucket {
> >>         spinlock_t      lock;
> >>         struct hlist_head       chain;
> >> };
> >>
> >> Am I missing something?
> >>
> > You're right, its not.  It seems to me that sctp_port_hashsize is meant to
> > simply bound the upper index of the hashtable array, and as such the phashfn
> > should not assume that its a power of 2 (i.e. it should simply mod the hash
> > value by sctp_port_hashsize rather than and-ing it).  Alternatively we could
> > simply use alloc_large_system_hash to allocate this hash table here, the way tcp
> > does.  I'm traveling right now, but can take care of this as soon as i get home
> > on wednesday
> 
> Hi Neil,
> 
> Thanks for confirming. It's yours, I don't pretend to fix it sooner.
> 
Copy that, I'll look at it later this week.  Thank you for the report
neil