netdev - Re: [PATCH RFC] net: introduce support for lazy initialization of secret keys

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: <20130925225827.GB30920@order.stressinduktion.org>
Date:	Thu, 26 Sep 2013 00:58:27 +0200
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, fw@...len.de, edumazet@...gle.com,
	davem@...emloft.net, ycheng@...gle.com
Subject: Re: [PATCH RFC] net: introduce support for lazy initialization of secret keys

On Wed, Sep 25, 2013 at 03:42:26PM -0700, Eric Dumazet wrote:
> On Wed, 2013-09-25 at 23:34 +0200, Hannes Frederic Sowa wrote:
> > net_get_random_once is a new macro which handles the initialization of
> > secret keys at use-site. It is possible to call it in the fast path. Only
> > the initialization depends on the spinlock and is rather slow. Otherwise
> > it should get used just before the key is used to delay the entropy
> > extration as late as possible to get better randomness. It returns true
> > if the key got initialized.
> 
> So you don't like cmpxchg() ;)

Actually, my first thought was to swap in a pointer via cmpxchg. But I didn't
know what to do when kmalloc returns NULL. After that I went with the spinlock
approach.

Also, I don't know with what size I get called. Consider this code:

| u8 byte;
| net_get_random_once(&byte, sizeof(byte));

Not allowing 0 in here would actually reduce the amount of randomness
considerable.

> 
> > +/* BE CAREFUL: this function is not interrupt safe */
> > +#define net_get_random_once(buf, nbytes)				\
> > +	({								\
> > +		static DEFINE_SPINLOCK(__lock);				\
> > +		static bool __done = false;				\
> > +		bool __ret = false;					\
> > +		if (unlikely(!__done))					\
> > +			__ret = __net_get_random_once(buf,		\
> > +						    nbytes,		\
> > +						    &__done,		\
> > +						    &__lock);		\
> > +		__ret;							\
> > +	})
> > +
> >  
> 
> No idea why its needed to have one spinlock per call point.
> 
> A single lock should be more than enough.
> 
> The spinlock could be private to __net_get_random_once()

Ack! Very good. Don't know why I build this so complex.

> +bool __net_get_random_once(void *buf, int nbytes, bool *done,
> +                           spinlock_t *lock)
> +{
> +       spin_lock_bh(lock);
> +       if (*done) {
> +               spin_unlock_bh(lock);
> +               return false;
> +       }
> +
> +       get_random_bytes(buf, nbytes);
> 
> I think you might need a memory barrier here.
> 
> (smp_wmb();)

I actually did some research on this and came to the conclusion that the
call to a function in another compilation unit should be barrier enough
(I really forgot to consider other architectures).

But a barrier should not hurt and it will be executed very infrequently,
so I'll add one.

> 
> +       *done = true;
> +       spin_unlock_bh(lock);
> 
> 
> BTW, build_ehash_secret() is called like that :
> 
> if (unlikely(!inet_ehash_secret))
>     if (sock->type != SOCK_RAW && sock->type != SOCK_DGRAM)
>         build_ehash_secret();
> 
> So it would be better to make sure inet_ehash_secret is not 0 by
> accident.

Urks. I'll think about that.

Thanks a lot for the review, Eric!

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html