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;
MIME-Version: 1.0
References: <CALFqKjSnOWyFjp7NQZKMXQ+TfzXMCBS=y8xnv5GE56SHVr5tCg@mail.gmail.com>
 <CACXcFmnRAkrj0Q3uFjyLu7RaWQh0VPbmErkj+cUaxZMb=YiGCw@mail.gmail.com> <CACXcFmmW+tCUf8JS=a=wJEnBY2JojP8VwEGLncYcGLZqiU+5Jw@mail.gmail.com>
In-Reply-To: <CACXcFmmW+tCUf8JS=a=wJEnBY2JojP8VwEGLncYcGLZqiU+5Jw@mail.gmail.com>
From:   Sandy Harris <sandyinchina@gmail.com>
Date:   Mon, 16 Aug 2021 18:59:50 +0800
Message-ID: <CACXcFmmosvNMxwjOFZ9SDadqJE11w6Vva+i9AV1zYQxbwoB9sA@mail.gmail.com>
Subject: Re: Lockless /dev/random - Performance/Security/Stability improvement
To:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        "Ted Ts'o" <tytso@mit.edu>, Stephan Mueller <smueller@chronox.de>,
        John Denker <jsd@av8n.com>, m@ib.tc
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

I wrote:

> > The basic ideas here look good to me; I will look at details later.
>
> Looking now, finding some things questionable.
> ...

I'm still looking & finding things that seem worth wondering about, &
asking about here.

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. I'd have:

/* Generate a hash across the pool, 16 words (512 bits) at a time */
    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 ...
     */
    spin_lock_irqsave(&r->lock, flags);
    __mix_pool_bytes(r, hash.w, sizeof(hash.w));
    spin_unlock_irqrestore(&r->lock, flags);