Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy

From: Stephan Müller
Date: Sun Nov 17 2019 - 17:57:12 EST

Next message: Paul Cercueil: "Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver"
Previous message: Iuliana Prodan: "[PATCH 00/12] crypto: caam - backlogging support"
In reply to: Andy Lutomirski: "Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy"
Next in thread: Andy Lutomirski: "Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Am Samstag, 16. November 2019, 17:51:24 CET schrieb Andy Lutomirski:

Hi Andy,

> > On Nov 16, 2019, at 1:40 AM, Stephan MÃller <smueller@xxxxxxxxxx> wrote:
> >
> > ïThe test interface allows a privileged process to capture the raw
> > unconditioned noise that is collected by the LRNG for statistical
> > analysis. Extracted noise data is not used to seed the LRNG. This
> > is a test interface and not appropriate for production systems.
> > Yet, the interface is considered to be sufficiently secured for
> > production systems.
> >
> > Access to the data is given through the lrng_raw debugfs file. The
> > data buffer should be multiples of sizeof(u32) to fill the entire
> > buffer. Using the option lrng_testing.boot_test=1 the raw noise of
> > the first 1000 entropy events since boot can be sampled.
> >
> > This test interface allows generating the data required for
> > analysis whether the LRNG is in compliance with SP800-90B
> > sections 3.1.3 and 3.1.4.
> >
> > CC: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> > CC: "Alexander E. Patrakov" <patrakov@xxxxxxxxx>
> > CC: "Ahmed S. Darwish" <darwish.07@xxxxxxxxx>
> > CC: "Theodore Y. Ts'o" <tytso@xxxxxxx>
> > CC: Willy Tarreau <w@xxxxxx>
> > CC: Matthew Garrett <mjg59@xxxxxxxxxxxxx>
> > CC: Vito Caputo <vcaputo@xxxxxxxxxxx>
> > CC: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> > CC: Jan Kara <jack@xxxxxxx>
> > CC: Ray Strode <rstrode@xxxxxxxxxx>
> > CC: William Jon McCann <mccann@xxxxxxx>
> > CC: zhangjs <zachary@xxxxxxxxxxxxxxxx>
> > CC: Andy Lutomirski <luto@xxxxxxxxxx>
> > CC: Florian Weimer <fweimer@xxxxxxxxxx>
> > CC: Lennart Poettering <mzxreary@xxxxxxxxxxx>
> > CC: Nicolai Stange <nstange@xxxxxxx>
> > Reviewed-by: Roman Drahtmueller <draht@xxxxxxxxxxxxxx>
> > Tested-by: Roman DrahtmÃller <draht@xxxxxxxxxxxxxx>
> > Tested-by: Marcelo Henrique Cerri <marcelo.cerri@xxxxxxxxxxxxx>
> > Tested-by: Neil Horman <nhorman@xxxxxxxxxx>
> > Signed-off-by: Stephan Mueller <smueller@xxxxxxxxxx>
> > ---
> > drivers/char/lrng/Kconfig | 16 ++
> > drivers/char/lrng/Makefile | 1 +
> > drivers/char/lrng/lrng_testing.c | 324 +++++++++++++++++++++++++++++++
> > 3 files changed, 341 insertions(+)
> > create mode 100644 drivers/char/lrng/lrng_testing.c
> >
> > diff --git a/drivers/char/lrng/Kconfig b/drivers/char/lrng/Kconfig
> > index e6ca3acc1e48..4ccc710832ef 100644
> > --- a/drivers/char/lrng/Kconfig
> > +++ b/drivers/char/lrng/Kconfig
> > @@ -169,4 +169,20 @@ config LRNG_APT_CUTOFF
> >
> > default 325 if !LRNG_APT_BROKEN
> > default 32 if LRNG_APT_BROKEN
> >
> > +config LRNG_TESTING
> > + bool "Enable entropy test interface to LRNG noise source"
> > + select CONFIG_DEBUG_FS
> > + help
> > + The test interface allows a privileged process to capture
> > + the raw unconditioned noise that is collected by the LRNG
> > + for statistical analysis. Extracted noise data is not used
> > + to seed the LRNG.
> > +
> > + The raw noise data can be obtained using the lrng_raw
> > + debugfs file. Using the option lrng_testing.boot_test=1
> > + the raw noise of the first 1000 entropy events since boot
> > + can be sampled.
> > +
> > + If unsure, say N.
> > +
> > endif # LRNG
> > diff --git a/drivers/char/lrng/Makefile b/drivers/char/lrng/Makefile
> > index 0713e9c0aa6e..c0b6cc4301fe 100644
> > --- a/drivers/char/lrng/Makefile
> > +++ b/drivers/char/lrng/Makefile
> > @@ -16,3 +16,4 @@ obj-$(CONFIG_LRNG_KCAPI) += lrng_kcapi.o
> > obj-$(CONFIG_LRNG_JENT) += lrng_jent.o
> > obj-$(CONFIG_LRNG_TRNG_SUPPORT) += lrng_trng.o
> > obj-$(CONFIG_LRNG_HEALTH_TESTS) += lrng_health.o
> > +obj-$(CONFIG_LRNG_TESTING) += lrng_testing.o
> > diff --git a/drivers/char/lrng/lrng_testing.c
> > b/drivers/char/lrng/lrng_testing.c new file mode 100644
> > index 000000000000..5c33d3bd2172
> > --- /dev/null
> > +++ b/drivers/char/lrng/lrng_testing.c
> > @@ -0,0 +1,324 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > +/*
> > + * Linux Random Number Generator (LRNG) Raw entropy collection tool
> > + *
> > + * Copyright (C) 2019, Stephan Mueller <smueller@xxxxxxxxxx>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/bug.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/module.h>
> > +#include <linux/sched.h>
> > +#include <linux/sched/signal.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/workqueue.h>
> > +#include <asm/errno.h>
> > +
> > +#include "lrng_internal.h"
> > +
> > +#define LRNG_TESTING_RINGBUFFER_SIZE 1024
> > +#define LRNG_TESTING_RINGBUFFER_MASK (LRNG_TESTING_RINGBUFFER_SIZE -
> > 1)
> > +
> > +static u32 lrng_testing_rb[LRNG_TESTING_RINGBUFFER_SIZE];
> > +static atomic_t lrng_rb_reader = ATOMIC_INIT(0);
> > +static atomic_t lrng_rb_writer = ATOMIC_INIT(0);
> > +static atomic_t lrng_rb_first_in = ATOMIC_INIT(0);
> > +static atomic_t lrng_testing_enabled = ATOMIC_INIT(0);
> > +
> > +static DECLARE_WAIT_QUEUE_HEAD(lrng_raw_read_wait);
> > +
> > +static u32 boot_test = 0;
> > +module_param(boot_test, uint, 0644);
> > +MODULE_PARM_DESC(boot_test, "Enable gathering boot time entropy of the
> > first" + " entropy events");
> > +
> > +static inline void lrng_raw_entropy_reset(void)
> > +{
> > + atomic_set(&lrng_rb_reader, 0);
> > + atomic_set(&lrng_rb_writer, 0);
> > + atomic_set(&lrng_rb_first_in, 0);
> > +}
> > +
> > +static void lrng_raw_entropy_init(void)
> > +{
> > + /*
> > + * The boot time testing implies we have a running test. If the
> > + * caller wants to clear it, he has to unset the boot_test flag
> > + * at runtime via sysfs to enable regular runtime testing
> > + */
> > + if (boot_test)
> > + return;
> > +
> > + lrng_raw_entropy_reset();
> > + atomic_set(&lrng_testing_enabled, 1);
> > + pr_warn("Enabling raw entropy collection\n");
> > +}
> > +
> > +static void lrng_raw_entropy_fini(void)
> > +{
> > + if (boot_test)
> > + return;
> > +
> > + lrng_raw_entropy_reset();
> > + atomic_set(&lrng_testing_enabled, 0);
> > + pr_warn("Disabling raw entropy collection\n");
> > +}
> > +
> > +bool lrng_raw_entropy_store(u32 value)
> > +{
> > + unsigned int write_ptr;
> > + unsigned int read_ptr;
> > +
> > + if (!atomic_read(&lrng_testing_enabled) && !boot_test)
> > + return false;
> > +
> > + write_ptr = (unsigned int)atomic_add_return_relaxed(1,
> > &lrng_rb_writer); + read_ptr = (unsigned
> > int)atomic_read(&lrng_rb_reader);

Before answering your comments, please allow me to clarify the following:

This entire code is intended to obtain take raw unconditioned noise data that
needs to be extracted from the kernel to user space to allow it to be further
analyzed. This is also why it is mentioned in the Kconfig selection that in
doubt one should select N and that this code is not intended for production
kernels although this code should be secure enough to be present in production
kernels.

For example, raw unconditioned noise data needs to be processed by the
complicated math outlined in chapter 6 of [1]. For that, there is a tool
available, see [2]. For that tool, data is needed that is obtained with the
getrawentropy tool available with [3] where this tool obtains the data from
the SysFS file that is implemented with this C file.

In addition, [1] even needs the data from the very first 1000 interrupts after
boot. Hence, the LRNG needs to be able to store that data until user space can
pick it up (see the boot_test variable).

The assessment resulting from this can be reviewed at [4] section 3.2 In
particular, the numbers provided at the end of sections 3.2.3 and 3.2.4 are
obtained with this interface.

Other examples where such raw unconditioned noise data is needed for further
analysis is [5], especially chapter 6.

This testing has nothing to do with the runtime testing provided with the
patch set 11. All data that ends up here is not available to the LRNG and will
not contribute to any entropy collection.

See the following:

static inline void lrng_time_process(void)
{
...
if (lrng_raw_entropy_store(now_time))
return;

bool lrng_raw_entropy_store(u32 value)
{
...
if (!atomic_read(&lrng_testing_enabled) && !boot_test)
return false;
...
return true;

>
> Am I correct in assuming that this function can be called concurrently in
> different threads or CPUs?

Yes, because it is called indirectly by add_interrupt_randomness.
> > +
> > + /*
> > + * Disable entropy testing for boot time testing after ring buffer
> > + * is filled.
> > + */
> > + if (boot_test && write_ptr > LRNG_TESTING_RINGBUFFER_SIZE) {
> > + pr_warn_once("Boot time entropy collection test disabled\n");
> > + return false;
> > + }
> > +
> > + if (boot_test && !atomic_read(&lrng_rb_first_in))
> > + pr_warn("Boot time entropy collection test enabled\n");
> > +
> > + lrng_testing_rb[write_ptr & LRNG_TESTING_RINGBUFFER_MASK] = value;
>
> Youâre writing *somewhere*, but not necessarily to the first open slot.

The idea is that there is a reader pointer and a writer pointer where the
reader always must be smaller or equal to the writer (modulo the size of the
ring buffer). So, I do not care where the writer ptr is.

All I need is that:

1. reader and writer ptr must start with the same value at boot time (e.g. 0)

2. reader ptr is always <= writer ptr in order for data to be read.

With these two conditions, when pulling data from the buffer, I need to pull
always the data from the reader ptr until the reader ptr reaches the writer
pointer.

Note, the reader/writer pointers are always set to 0 at the beginning of a new
read request from user space.

>
> > +
> > + /* We got at least one event, enable the reader now. */
> > + atomic_set(&lrng_rb_first_in, 1);
>
> But not necessarily in position 0.

Yes, this is perfectly ok.
>
> > +
> > + if (wq_has_sleeper(&lrng_raw_read_wait))
> > + wake_up_interruptible(&lrng_raw_read_wait);
> > +
> > + /*
> > + * Our writer is taking over the reader - this means the reader
> > + * one full ring buffer available. Thus we "push" the reader ahead
> > + * to guarantee that he will be able to consume the full ring.
> > + */
> > + if (!boot_test &&
> > + ((write_ptr & LRNG_TESTING_RINGBUFFER_MASK) ==
> > + (read_ptr & LRNG_TESTING_RINGBUFFER_MASK)))
> > + atomic_inc_return_relaxed(&lrng_rb_reader);
>
> Because you did a relaxed increment above, you donât actually know this.
> Maybe itâs okay, but this is way too subtle.

You are absolutely correct, there should be no relaxed atomic operation. We
should take the atomic_inc above and here.

I fixed this.
>
> I think you should have a mutex for the read side and put all the
> complicated accounting inside the mutex. If the reader canât figure out
> that the read pointer is too far behind the write pointer, then fix the
> reader.

Done - the writer now only writes the data and generates the boot log if the
boot time raw entropy gathering is enabled.
>
> I also donât see how the reader is supposed to know how much data has
> actually been written. You donât have any variable that says âall words up
> to X have been writtenâ.

With the two rules above, I think the reader knows that: all data between the
reader ptr and the writer ptr modulo the size of the ring buffer.

But I simplified the code now, the code now only copies the data out if the
reader <= writer modulo the ring buffer size. In this case, if the writer is
much faster, then we loose some values.

With the old code, we simply would have lost it too, but just a bit later.
>
> I think you should stop trying to make the write side wait free.
> Instead,
> consider either using a lock or making it unreliable. For the former, just
> skip taking the lock if testing is off. For the latter, read write_ptr,
> write (using WRITE_ONCE) your data, then cmpxchg the write ptr from the
> value you read to that value plus one. And make sure that the reader never
> tries to read the first unwritten slot, i.e. never let the reader catch all
> the way up.

I have followed the locking approach as we need to get correct data.
>
> Iâm also curious why you need entirely different infrastructure for testing
> as for normal operation.

I hope with the explanation above, the question is answered.

> > +
> > + return true;
> > +}
> > +
> > +static inline bool lrng_raw_have_data(void)
> > +{
> > + unsigned int read_ptr = (unsigned int)atomic_read(&lrng_rb_reader);
> > + unsigned int write_ptr = (unsigned int)atomic_read(&lrng_rb_writer);
> > +
> > + return (atomic_read(&lrng_rb_first_in) &&
> > + (write_ptr & LRNG_TESTING_RINGBUFFER_MASK) !=
> > + (read_ptr & LRNG_TESTING_RINGBUFFER_MASK));
> > +}
> > +
> > +static int lrng_raw_entropy_reader(u8 *outbuf, u32 outbuflen)
> > +{
> > + int collected_data = 0;
> > +
> > + if (!atomic_read(&lrng_testing_enabled) && !boot_test)
> > + return -EAGAIN;
> > +
> > + if (!atomic_read(&lrng_rb_first_in)) {
> > + wait_event_interruptible(lrng_raw_read_wait,
> > + lrng_raw_have_data());
> > + if (signal_pending(current))
> > + return -ERESTARTSYS;
> > + }
> > +
> > + while (outbuflen) {
> > + unsigned int read_ptr =
> > + (unsigned int)atomic_add_return_relaxed(
> > + 1, &lrng_rb_reader);
> > + unsigned int write_ptr =
> > + (unsigned int)atomic_read(&lrng_rb_writer);
> > +
> > + /*
> > + * For boot time testing, only output one round of ring buffer.
> > + */
> > + if (boot_test && read_ptr > LRNG_TESTING_RINGBUFFER_SIZE) {
> > + collected_data = -ENOMSG;
> > + goto out;
> > + }
> > +
> > + /* We reached the writer */
> > + if (!boot_test && ((write_ptr & LRNG_TESTING_RINGBUFFER_MASK) ==
> > + (read_ptr & LRNG_TESTING_RINGBUFFER_MASK))) {
> > +
>
> This is wrong. The fact that you havenât reached the writer does not imply
> that youâre about to read valid data.

As I changed the code by using your locking suggestion. With that I think the
code should now always read correct data. I will send an updated patch set
tomorrow.

Thank you for your review.

[1] https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90B.pdf

[2] https://github.com/usnistgov/SP800-90B_EntropyAssessment

[3] http://www.chronox.de/lrng/lrng-tests-20191116.tar.xz - see the sp80090b
directory for details

[4] http://www.chronox.de/lrng/doc/lrng.pdf

[5] https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/
LinuxRNG/LinuxRNG_EN.pdf

Ciao
Stephan

Next message: Paul Cercueil: "Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver"
Previous message: Iuliana Prodan: "[PATCH 00/12] crypto: caam - backlogging support"
In reply to: Andy Lutomirski: "Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy"
Next in thread: Andy Lutomirski: "Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]