Message-ID: <CALCETrUm5gM9s6vqvXWeT5phwRnh-CgLhYY=_eNZ2FDVjBAxSw@mail.gmail.com>
Date:   Tue, 19 Nov 2019 02:04:42 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Stephan Müller <smueller@...onox.de>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        "Alexander E. Patrakov" <patrakov@...il.com>,
        "Ahmed S. Darwish" <darwish.07@...il.com>,
        "Theodore Y. Ts'o" <tytso@....edu>, Willy Tarreau <w@....eu>,
        Matthew Garrett <mjg59@...f.ucam.org>,
        Vito Caputo <vcaputo@...garu.com>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Jan Kara <jack@...e.cz>, Ray Strode <rstrode@...hat.com>,
        William Jon McCann <mccann@....edu>,
        zhangjs <zachary@...shancloud.com>,
        Andy Lutomirski <luto@...nel.org>,
        Florian Weimer <fweimer@...hat.com>,
        Lennart Poettering <mzxreary@...inter.de>,
        Nicolai Stange <nstange@...e.de>,
        "Peter, Matthias" <matthias.peter@....bund.de>,
        Marcelo Henrique Cerri <marcelo.cerri@...onical.com>,
        Roman Drahtmueller <draht@...altsekun.de>,
        Neil Horman <nhorman@...hat.com>
Subject: Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy

On Sun, Nov 17, 2019 at 2:55 PM Stephan Müller <smueller@...onox.de> wrote:
>
> 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@...onox.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@...ssion.com>
> > > CC: "Alexander E. Patrakov" <patrakov@...il.com>
> > > CC: "Ahmed S. Darwish" <darwish.07@...il.com>
> > > CC: "Theodore Y. Ts'o" <tytso@....edu>
> > > CC: Willy Tarreau <w@....eu>
> > > CC: Matthew Garrett <mjg59@...f.ucam.org>
> > > CC: Vito Caputo <vcaputo@...garu.com>
> > > CC: Andreas Dilger <adilger.kernel@...ger.ca>
> > > CC: Jan Kara <jack@...e.cz>
> > > CC: Ray Strode <rstrode@...hat.com>
> > > CC: William Jon McCann <mccann@....edu>
> > > CC: zhangjs <zachary@...shancloud.com>
> > > CC: Andy Lutomirski <luto@...nel.org>
> > > CC: Florian Weimer <fweimer@...hat.com>
> > > CC: Lennart Poettering <mzxreary@...inter.de>
> > > CC: Nicolai Stange <nstange@...e.de>
> > > Reviewed-by: Roman Drahtmueller <draht@...altsekun.de>
> > > Tested-by: Roman Drahtmüller <draht@...altsekun.de>
> > > Tested-by: Marcelo Henrique Cerri <marcelo.cerri@...onical.com>
> > > Tested-by: Neil Horman <nhorman@...hat.com>
> > > Signed-off-by: Stephan Mueller <smueller@...onox.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@...onox.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);
>
> 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:
>
>


>         if (this_cpu_read(cpu_tlbstate.loaded_mm) == &init_mm)
>                 return;
>
> --
> 2.17.1
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.

You're making an implicit assumption, though: that the writer has
actually written the data indicated by the writer ptr.  But you're
updating the writer ptr *before* you write, so that assumption is
false.

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

I'll look at the new code.  I'm not sure what you mean by a <= b
modulo N -- in a cyclic group (e.g. the integers 0 .. N-1), there
isn't really a well-defined concept of <=.

Powered by blists - more mailing lists