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@xxxxxxxxxxxxxx> 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. 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. thx, Jason. > >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > >> --- > >> 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@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Kees Cook > Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
