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

[Andy Lutomirski <luto@amacapital.net>]

> On Nov 16, 2019, at 1:40 AM, Stephan Müller <smueller@chronox.de> 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@xmission.com>
> CC: "Alexander E. Patrakov" <patrakov@gmail.com>
> CC: "Ahmed S. Darwish" <darwish.07@gmail.com>
> CC: "Theodore Y. Ts'o" <tytso@mit.edu>
> CC: Willy Tarreau <w@1wt.eu>
> CC: Matthew Garrett <mjg59@srcf.ucam.org>
> CC: Vito Caputo <vcaputo@pengaru.com>
> CC: Andreas Dilger <adilger.kernel@dilger.ca>
> CC: Jan Kara <jack@suse.cz>
> CC: Ray Strode <rstrode@redhat.com>
> CC: William Jon McCann <mccann@jhu.edu>
> CC: zhangjs <zachary@baishancloud.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Florian Weimer <fweimer@redhat.com>
> CC: Lennart Poettering <mzxreary@0pointer.de>
> CC: Nicolai Stange <nstange@suse.de>
> Reviewed-by: Roman Drahtmueller <draht@schaltsekun.de>
> Tested-by: Roman Drahtmüller <draht@schaltsekun.de>
> Tested-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> Tested-by: Neil Horman <nhorman@redhat.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
> 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@chronox.de>
> + */
> +
> +#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);

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

> +
> +    /*
> +     * 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.

> +
> +    /* We got at least one event, enable the reader now. */
> +    atomic_set(&lrng_rb_first_in, 1);

But not necessarily in position 0.

> +
> +    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.

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.

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”.

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’m also curious why you need entirely different infrastructure for testing as for normal operation.

> +
> +    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.