Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 16 Aug 2021 10:25:37 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     Sandy Harris <sandyinchina@gmail.com>
Cc:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Stephan Mueller <smueller@chronox.de>,
        John Denker <jsd@av8n.com>, m@ib.tc
Subject: Re: Lockless /dev/random - Performance/Security/Stability improvement
Message-ID: <YRp1YbpFdNG0IJMI@mit.edu>
References: <CALFqKjSnOWyFjp7NQZKMXQ+TfzXMCBS=y8xnv5GE56SHVr5tCg@mail.gmail.com>
 <CACXcFmnRAkrj0Q3uFjyLu7RaWQh0VPbmErkj+cUaxZMb=YiGCw@mail.gmail.com>
 <CACXcFmmW+tCUf8JS=a=wJEnBY2JojP8VwEGLncYcGLZqiU+5Jw@mail.gmail.com>
 <CACXcFmmosvNMxwjOFZ9SDadqJE11w6Vva+i9AV1zYQxbwoB9sA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACXcFmmosvNMxwjOFZ9SDadqJE11w6Vva+i9AV1zYQxbwoB9sA@mail.gmail.com>
Precedence: bulk

On Mon, Aug 16, 2021 at 06:59:50PM +0800, Sandy Harris wrote:
> I am by no means convinced that Mike's idea of a lockless driver is a
> good one. Without some locks, if two or more parts of the code write
> to the same data structure, then there's a danger one will overwrite
> the other's contribution & we'll lose entropy.
> 
> However, I cannot see why any data structure should be locked when it
> is only being read. There's no reason to care if others read it as
> well. If someone writes to it, then the result of reading becomes
> indeterminate. In most applications, that would be a very Bad Thing.
> In this contact, though, it is at worst harmless & possibly a Good
> Thing because it would make some attacks harder.
> 
> For example, looking at the 5.8.9 kernel Ubuntu gives me, I find this
> in xtract_buf()
> 
> /* Generate a hash across the pool, 16 words (512 bits) at a time */
>     spin_lock_irqsave(&r->lock, flags);
>     for (i = 0; i < r->poolinfo->poolwords; i += 16)
>         sha1_transform(hash.w, (__u8 *)(r->pool + i), workspace);
> 
>     /*
>      * We mix the hash back into the pool ...
>      */
>     __mix_pool_bytes(r, hash.w, sizeof(hash.w));
>     spin_unlock_irqrestore(&r->lock, flags);
> 
> The lock is held throughout the fairly expensive hash operation & I
> see no reason why it should be.

The reason why this is there is because otherwise, there can be two
processes both trying to extract entry from the pool, and getting the
same result, and returning the identical "randomness" to two different
userspace processes.  Which would be sad....  (unless you are a
nation-state attacker, I suppose :-)

Cheers,

					- Ted