From: Kees Cook Subject: Re: [PATCH][RESEND 3] hwrng: add randomness to system from rng sources Date: Wed, 5 Mar 2014 13:51:24 -0800 Message-ID: References: <20140303235148.GA7601@www.outflux.net> <20140304153841.GN1872@titan.lakedaemon.net> <20140304195356.GS1872@titan.lakedaemon.net> <1393972797.8344.190.camel@calx> <20140305211145.GV1872@titan.lakedaemon.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Matt Mackall , "Theodore Ts'o" , LKML , Herbert Xu , Rusty Russell , Satoru Takeuchi , linux-crypto , Andrew Morton To: Jason Cooper Return-path: Received: from mail-oa0-f45.google.com ([209.85.219.45]:41706 "EHLO mail-oa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755556AbaCEVvZ (ORCPT ); Wed, 5 Mar 2014 16:51:25 -0500 Received: by mail-oa0-f45.google.com with SMTP id o6so1700538oag.32 for ; Wed, 05 Mar 2014 13:51:24 -0800 (PST) In-Reply-To: <20140305211145.GV1872@titan.lakedaemon.net> Sender: linux-crypto-owner@vger.kernel.org List-ID: On Wed, Mar 5, 2014 at 1:11 PM, Jason Cooper wrote: > Matt, Kees, > > On Tue, Mar 04, 2014 at 04:39:57PM -0600, Matt Mackall wrote: >> On Tue, 2014-03-04 at 11:59 -0800, Kees Cook wrote: >> > On Tue, Mar 4, 2014 at 11:53 AM, Jason Cooper wrote: >> > > On Tue, Mar 04, 2014 at 11:01:49AM -0800, Kees Cook wrote: >> > >> On Tue, Mar 4, 2014 at 7:38 AM, Jason Cooper wrote: >> > >> > On Mon, Mar 03, 2014 at 03:51:48PM -0800, Kees Cook wrote: > ... >> > Well, I think there's confusion here over "the" hwrng and "a" hwrng. I >> > have devices with multiple entropy sources, and all my hwrngs are >> > built as modules, so I choose when to load them into my kernel. "The" >> > arch-specific entropy source (e.g. RDRAND) is very different. > > Dynamically loading modules _after_ boot is fine. Especially with > Matt's clear explanation. I'm not concerned about that scenario. > >> > > Please understand, my point-of-view is as someone who installs Linux on >> > > equipment *after* purchase (hobbyist, tinkers). If I control the part >> > > selection and sourcing of the board components, of course I have more >> > > trust in the hwrng. >> > > >> > > So my situation is similar to buying an Intel based laptop. I can't do >> > > a special order at Bestbuy and ask for a system without the RDRAND >> > > instruction. Same with the hobbyist market. We buy the gear, but we >> > > have no control over what's inside it. >> > > >> > > In that situation, without this patch, I would enable the hwrng for the >> > > board. With the patch in it's current form, I would start looking for >> > > research papers and discussions regarding using the hwrng for input. If >> > > the patch provided arch_get_random_long(), I would feel comfortable >> > > enabling the hwrng. >> > > >> > > Perhaps I'm being too conservative, but I'd rather have the discussion >> > > now and have concerns proven unfounded than have someone say "How the >> > > hell did this happen?" three releases down the road. >> > >> > Sure, and I don't want to be the one weakening the entropy pool. >> >> [temporarily coming out of retirement to provide a clue] > > Thanks, Matt! > >> The pool mixing function is intentionally _reversible_. This is a >> crucial security property. >> >> That means, if I have an initial secret pool state X, and hostile >> attacker controlled data Y, then we can do: >> >> X' = mix(X, Y) >> >> and >> >> X = unmix(X', Y) >> >> We can see from this that the combination of (X' and Y) still contain >> the information that was originally in X. Since it's clearly not in Y.. >> it must all remain in X'. >> >> In other words, if there are 4096 bits of "unknownness" in X to start >> with, and I can get those same 4096 bits of "unknownness" back by >> unmixing X' and Y, then there must still be 4096 bits of "unknownness" >> in X'. If X' is 4096 bits long, then we've just proven that >> reversibility means the attacker can know nothing about the contents of >> X' by his choice of Y. > > Well, this reinforces my comfortability with loadable modules. The pool > is already initialized by the point at which the driver is loaded. > > Unfortunately, any of the drivers in hw_random can be built in. When > built in, hwrng_register is going to be called during the kernel > initialization process. In that case, the unknownness in X is not 4096 > bits, but far less. Also, the items that may have seeded X (MAC addr, > time, etc) are discoverable by a potential attacker. This is also well > before random-seed has been fed in. Would reducing the size of the buffer used for this help your concerns? Ironically, a non-malicious hwng using this code would actually improve defense against other early-boot rng badness since unlike time and MAC, this is much less discoverable by an attacker. > > That's the crux of my concern with this patch. Basically, the user can > choose 'y' over 'm', say when building a dedicated system w/o loadable > modules, and not realize how much more trust he has placed in the hwrng. > > How does this patch look? I'm not claiming my change is acceptable for > mainline, just seeking feedback on the concept. IS_MODULE() really > doesn't have many users afaict. Regardless, making this change to the patch would be fine with me, yeah. I think the module case is much more common. -Kees > > This builds without warning on ARCH=arm with multi_v7_defconfig when > HW_RANDOM={y,m,n} > > thx, > > Jason. > > -------->8-------------------------- > commit 0fcc0669ee4bbeae355c0f61f79a6b319a32ba12 > Author: Kees Cook > Date: Mon Mar 3 15:51:48 2014 -0800 > > hwrng: add randomness to system from rng sources > > When bringing a new RNG source online, it seems like it would make sense > to use some of its bytes to make the system entropy pool more random, > as done with all sorts of other devices that contain per-device or > per-boot differences. > > [jac] prevent the code from being called at boot up, before the entropy > pool has had time to build and be well initialized. We do this by > making the code conditional on being built as a module. > > Signed-off-by: Kees Cook > Signed-off-by: Jason Cooper > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index a0f7724852eb..5c3314cbf671 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -41,6 +41,8 @@ > #include > #include > #include > +#include > +#include > #include > > > @@ -305,6 +307,10 @@ int hwrng_register(struct hwrng *rng) > int must_register_misc; > int err = -EINVAL; > struct hwrng *old_rng, *tmp; > +#if IS_MODULE(CONFIG_HW_RANDOM) > + unsigned char bytes[16]; > + int bytes_read; > +#endif > > if (rng->name == NULL || > (rng->data_read == NULL && rng->read == NULL)) > @@ -348,6 +354,18 @@ int hwrng_register(struct hwrng *rng) > } > INIT_LIST_HEAD(&rng->list); > list_add_tail(&rng->list, &rng_list); > + > + /* > + * Out of an abundance of caution, we should avoid seeding the entropy > + * pool when it is first initialized (ie at kernel boot time). > + * Therefore, we take advantage of the hwrng when it is built as a > + * module, but not when built in. > + */ > +#if IS_MODULE(CONFIG_HW_RANDOM) > + bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1); > + if (bytes_read > 0) > + add_device_randomness(bytes, bytes_read); > +#endif > out_unlock: > mutex_unlock(&rng_mutex); > out: -- Kees Cook Chrome OS Security