Message-ID: <CAGXu5jL5JBHL0tqZzA03LY67HZU4jEQ0_V=bJs9TV3g0Bfwr_w@mail.gmail.com>
Date:	Tue, 4 Mar 2014 11:59:41 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Jason Cooper <jason@...edaemon.net>
Cc:	"Theodore Ts'o" <tytso@....edu>,
	LKML <linux-kernel@...r.kernel.org>,
	Matt Mackall <mpm@...enic.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Satoru Takeuchi <satoru.takeuchi@...il.com>,
	linux-crypto <linux-crypto@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH][RESEND 3] hwrng: add randomness to system from rng sources

On Tue, Mar 4, 2014 at 11:53 AM, Jason Cooper <jason@...edaemon.net> 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 <jason@...edaemon.net> wrote:
>> > Kees, Ted,
>> >
>> > On Mon, Mar 03, 2014 at 03:51:48PM -0800, Kees Cook wrote:
>> >> 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.
>> >
>> > Why is this necessary?  init_std_data() already calls
>> > arch_get_random_long() while stirring each of the pools.
>>
>> I may be misunderstanding something here, but hwrng isn't going to get
>> hit by a arch_get_random_long().
>
> ahh, you are correct.  It appears it's only used on x86 and powerpc.
> Bad assumption on my part.
>
>> That's just for arch-specific RNGs (e.g. RDRAND), where as hwrng is
>> for, effectively, add-on devices (e.g. TPMs).
>>
>> > I'm a little concerned here because this gives potentially untrusted
>> > hwrngs more influence over the entropy pools initial state than most
>> > users of random.c expect.  Many of the drivers in hw_random/ are
>> > platform drivers and are initialized before random.c.
>> >
>> > I'm comfortable with the design decisions Ted has made wrt random.c and
>> > hwrngs.  However, I think that this changes that trust relationship in a
>> > fundamental way.  I'm ok with building support into my kernels for
>> > hwrngs as long as random.c's internal use of them is limited to the
>> > mixing in extract_buf() and init_std_data().
>> >
>> > By adding this patch, even without crediting entropy to the pool, a
>> > rogue hwrng now has significantly more influence over the initial state
>> > of the entropy pools.  Or, am I missing something?
>>
>> I wasn't viewing this as dealing with rouge hwrngs (though shouldn't
>> that state still be covered due to the existing mixing), but more as a
>> "hey this thing has some randomness associated with it", similar to
>> the mixing done for things like NIC MAC, etc. (Better, actually, since
>> NIC MAC is going to be the same every boot.) It seemed silly to ignore
>> an actual entropy source when seeding.
>
> Agreed, but I think we need to be careful about how random.c interacts
> with any hwrng.  Ideally, the drivers in hw_random/ could provide
> arch_get_random_long().  This way, random.c still determines when and
> how to use the hwrng.
>
> Ultimately, the user (person compiling the kernel) will decide to trust
> or not trust the hwrng by enabling support for it or not.  My concern
> with this patch is that it changes the magnitude of that trust decision.
> And only the most diligent user would discover the change.
>
> To date, all discussion wrt random.c and hwrngs are that the output of
> the hwrng (in particular, RDRAND) is XORd with the output of the mixer.
> Now, we're saying it can provide input as well.

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.

>
> 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.
However, I think this patch is no different from the ones that stuff a
NIC MAC into the pool -- it's taking something from my system that is
unique or random and pushing the entropy seed around. It seems silly
that if I've loaded the hwrng-tpm module, my system entropy pool isn't
bumped.

-Kees

>
> thx,
>
> Jason.
>
>
>> >> Signed-off-by: Kees Cook <keescook@...omium.org>
>> >> ---
>> >>  drivers/char/hw_random/core.c |    7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> >> index a0f7724852eb..6e5bb68a708c 100644
>> >> --- a/drivers/char/hw_random/core.c
>> >> +++ b/drivers/char/hw_random/core.c
>> >> @@ -41,6 +41,7 @@
>> >>  #include <linux/miscdevice.h>
>> >>  #include <linux/delay.h>
>> >>  #include <linux/slab.h>
>> >> +#include <linux/random.h>
>> >>  #include <asm/uaccess.h>
>> >>
>> >>
>> >> @@ -305,6 +306,8 @@ int hwrng_register(struct hwrng *rng)
>> >>       int must_register_misc;
>> >>       int err = -EINVAL;
>> >>       struct hwrng *old_rng, *tmp;
>> >> +     unsigned char bytes[16];
>> >> +     int bytes_read;
>> >>
>> >>       if (rng->name == NULL ||
>> >>           (rng->data_read == NULL && rng->read == NULL))
>> >> @@ -348,6 +351,10 @@ int hwrng_register(struct hwrng *rng)
>> >>       }
>> >>       INIT_LIST_HEAD(&rng->list);
>> >>       list_add_tail(&rng->list, &rng_list);
>> >> +
>> >> +     bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
>> >> +     if (bytes_read > 0)
>> >> +             add_device_randomness(bytes, bytes_read);
>> >>  out_unlock:
>> >>       mutex_unlock(&rng_mutex);
>> >>  out:
>> >> --
>> >> 1.7.9.5
>> >>
>> >>
>> >> --
>> >> Kees Cook
>> >> Chrome OS Security
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>> >> the body of a message to majordomo@...r.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists