linux-kernel - Re: [PATCH RESEND] random: use correct memory barriers for crng_node

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: <CAHmME9rv9RZai-0diV6kdc9yfXRog29QiStEzDpC9v25OWY81Q@mail.gmail.com>
Date:   Mon, 20 Dec 2021 22:45:15 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     "Paul E . McKenney" <paulmck@...nel.org>
Cc:     Eric Biggers <ebiggers@...nel.org>,
        "Theodore Ts'o" <tytso@....edu>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH RESEND] random: use correct memory barriers for crng_node_pool

Hi Paul,

On Mon, Dec 20, 2021 at 8:00 PM Paul E. McKenney <paulmck@...nel.org> wrote:
> This assumes that the various crng_node_pool[i] pointers never change
> while accessible to readers (and that some sort of synchronization applies
> to the values in the pointed-to structure).  If these pointers do change,
> then there also needs to be a READ_ONCE(pool[nid]) in select_crng(), where
> the value returned from this READ_ONCE() is both tested and returned.
> (As in assign this value to a temporary.)
>
> But if the various crng_node_pool[i] pointers really are constant
> while readers can access them, then the cmpxchg_release() suffices.
> The loads from pool[nid] are then data-race free, and because they
> are unmarked, the compiler is prohibited from hoisting them out from
> within the "if" statement.  The address dependency prohibits the
> CPU from reordering them.

Right, this is just an initialization-time allocation and assignment,
never updated or freed again after.

> So READ_ONCE() should be just fine.  Which answers Jason's question.  ;-)

Great. So v2 of this patch can use READ_ONCE then. Thanks!

> Looking at _extract_crng(), if this was my code, I would use READ_ONCE()
> in the checks, but that might be my misunderstanding boot-time constraints
> or some such.  Without some sort of constraint, I don't see how the code
> avoids confusion from reloads of crng->init_time if two CPUs concurrently
> see the expiration of CRNG_RESEED_INTERVAL, but I could easily be missing
> something that makes this safe.  (And this is irrelevant to this patch.)

Indeed init_time seems to race via the crng_reseed path, and
READ_ONCE()ing that seems reasonable. The other setters of it --
initialize_{primary,secondary} -- are in the boot path.

Jason