| Message ID | 5778e65d5ca52bebbaa023e177d863e44f098e96.1405546879.git.luto@amacapital.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/16/2014 02:45 PM, Andy Lutomirski wrote: > diff --git a/arch/x86/include/asm/archslowrng.h b/arch/x86/include/asm/archslowrng.h > new file mode 100644 > index 0000000..c8e8d0d > --- /dev/null > +++ b/arch/x86/include/asm/archslowrng.h > @@ -0,0 +1,30 @@ > +/* > + * This file is part of the Linux kernel. > + * > + * Copyright (c) 2014 Andy Lutomirski > + * Authors: Andy Lutomirski <luto@amacapital.net> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > + > +#ifndef ASM_X86_ARCHSLOWRANDOM_H > +#define ASM_X86_ARCHSLOWRANDOM_H > + > +#ifndef CONFIG_ARCH_SLOW_RNG > +# error archslowrng.h should not be included if !CONFIG_ARCH_SLOW_RNG > +#endif > + I'm *seriously* questioning the wisdom of this. A much saner thing would be to do: #ifndef CONFIG_ARCH_SLOW_RNG /* Not supported */ static inline int arch_get_slow_rng_u64(u64 *v) { (void)v; return 0; } #endif ... which is basically what we do for the archrandom stuff. I'm also wondering if it makes sense to have a function which prefers arch_get_random*() over this one as a preferred interface. Something like: int get_random_arch_u64_slow_ok(u64 *v) { int i; u64 x = 0; unsigned long l; for (i = 0; i < 64/BITS_PER_LONG; i++) { if (!arch_get_random_long(&l)) return arch_get_slow_rng_u64(v); x |= l << (i*BITS_PER_LONG); } *v = l; return 0; } This still doesn't address the issue e.g. on x86 where RDRAND is available but we haven't set up alternatives yet. So it might be that what we really want is to encapsulate this fallback in arch code and do a more direct enumeration. > + > +static int kvm_get_slow_rng_u64(u64 *v) > +{ > + /* > + * Allow migration from a hypervisor with the GET_RNG_SEED > + * feature to a hypervisor without it. > + */ > + if (rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0) > + return 1; > + else > + return 0; > +} How about: return rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0; The naming also feels really inconsistent... -hpa -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 16, 2014 at 2:59 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 07/16/2014 02:45 PM, Andy Lutomirski wrote: >> diff --git a/arch/x86/include/asm/archslowrng.h b/arch/x86/include/asm/archslowrng.h >> new file mode 100644 >> index 0000000..c8e8d0d >> --- /dev/null >> +++ b/arch/x86/include/asm/archslowrng.h >> @@ -0,0 +1,30 @@ >> +/* >> + * This file is part of the Linux kernel. >> + * >> + * Copyright (c) 2014 Andy Lutomirski >> + * Authors: Andy Lutomirski <luto@amacapital.net> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + */ >> + >> +#ifndef ASM_X86_ARCHSLOWRANDOM_H >> +#define ASM_X86_ARCHSLOWRANDOM_H >> + >> +#ifndef CONFIG_ARCH_SLOW_RNG >> +# error archslowrng.h should not be included if !CONFIG_ARCH_SLOW_RNG >> +#endif >> + > > I'm *seriously* questioning the wisdom of this. A much saner thing > would be to do: > > #ifndef CONFIG_ARCH_SLOW_RNG > > /* Not supported */ > static inline int arch_get_slow_rng_u64(u64 *v) > { > (void)v; > return 0; > } > > #endif > > ... which is basically what we do for the archrandom stuff. The archrandom stuff defines the "not supported" variant in the generic header, which is what I'm doing here. I could wrap all of asm/archslowrng.h in #ifdef CONFIG_ARCH_SLOW_RNG instead of putting the #error in there, but I have no strong preference. > > I'm also wondering if it makes sense to have a function which prefers > arch_get_random*() over this one as a preferred interface. Something like: > > int get_random_arch_u64_slow_ok(u64 *v) > { > int i; > u64 x = 0; > unsigned long l; > > for (i = 0; i < 64/BITS_PER_LONG; i++) { > if (!arch_get_random_long(&l)) > return arch_get_slow_rng_u64(v); > > x |= l << (i*BITS_PER_LONG); > } > *v = l; > return 0; > } I played with something like this earlier, but I dropped it when it ended up having exactly one user. I suspect that the highly paranoid will actually prefer seeding with both sources in init_std_data even if RDRAND is available -- it costs very little and it provides a bit of extra assurance. > > This still doesn't address the issue e.g. on x86 where RDRAND is > available but we haven't set up alternatives yet. So it might be that > what we really want is to encapsulate this fallback in arch code and do > a more direct enumeration. My personal preference is to defer this until some user shows up. I think that even this would be too complicated for KASLR, which is the only extremely early-boot user that I found. Hmm. Does the prandom stuff want to use this? > >> + >> +static int kvm_get_slow_rng_u64(u64 *v) >> +{ >> + /* >> + * Allow migration from a hypervisor with the GET_RNG_SEED >> + * feature to a hypervisor without it. >> + */ >> + if (rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0) >> + return 1; >> + else >> + return 0; >> +} > > How about: > > return rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0; > > The naming also feels really inconsistent... Better ideas welcome. I could call the generic function arch_get_pv_random_seed, but maybe someone will come up with a non-paravirt implementation. --Andy > > -hpa >
On Wed, Jul 16, 2014 at 3:13 PM, Andy Lutomirski <luto@amacapital.net> wrote: > My personal preference is to defer this until some user shows up. I > think that even this would be too complicated for KASLR, which is the > only extremely early-boot user that I found. > > Hmm. Does the prandom stuff want to use this? prandom isn't even using rdrand. I'd suggest fixing this separately, or even just waiting until someone goes and deletes prandom. --Andy -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/16/2014 03:40 PM, Andy Lutomirski wrote: > On Wed, Jul 16, 2014 at 3:13 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> My personal preference is to defer this until some user shows up. I >> think that even this would be too complicated for KASLR, which is the >> only extremely early-boot user that I found. >> >> Hmm. Does the prandom stuff want to use this? > > prandom isn't even using rdrand. I'd suggest fixing this separately, > or even just waiting until someone goes and deletes prandom. > prandom is exactly the opposite; it is designed for when we need possibly low quality random numbers very quickly. RDRAND is actually too slow. -hpa -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jul 16, 2014 4:00 PM, "H. Peter Anvin" <hpa@zytor.com> wrote: > > On 07/16/2014 03:40 PM, Andy Lutomirski wrote: > > On Wed, Jul 16, 2014 at 3:13 PM, Andy Lutomirski <luto@amacapital.net> wrote: > >> My personal preference is to defer this until some user shows up. I > >> think that even this would be too complicated for KASLR, which is the > >> only extremely early-boot user that I found. > >> > >> Hmm. Does the prandom stuff want to use this? > > > > prandom isn't even using rdrand. I'd suggest fixing this separately, > > or even just waiting until someone goes and deletes prandom. > > > > prandom is exactly the opposite; it is designed for when we need > possibly low quality random numbers very quickly. RDRAND is actually > too slow. I meant that prandom isn't using rdrand for early seeding. --Andy > > -hpa > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/16/2014 05:03 PM, Andy Lutomirski wrote: >> >> prandom is exactly the opposite; it is designed for when we need >> possibly low quality random numbers very quickly. RDRAND is actually >> too slow. > > I meant that prandom isn't using rdrand for early seeding. > We should probably fix that. -hpa -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 16, 2014 at 09:55:15PM -0700, H. Peter Anvin wrote: > On 07/16/2014 05:03 PM, Andy Lutomirski wrote: > >> > > I meant that prandom isn't using rdrand for early seeding. > > > > We should probably fix that. It wouldn't hurt to explicitly use arch_get_random_long() in prandom, but it does use get_random_bytes() in early seed, and for CPU's with RDRAND present, we do use it in init_std_data() in drivers/char/random.c, so prandom is already getting initialized via an RNG (which is effectively a DRBG even if it doesn't pass all of NIST's rules) which is derived from RDRAND. Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/17/2014 12:59 AM, H. Peter Anvin wrote: > On 07/16/2014 03:40 PM, Andy Lutomirski wrote: >> On Wed, Jul 16, 2014 at 3:13 PM, Andy Lutomirski <luto@amacapital.net> wrote: >>> My personal preference is to defer this until some user shows up. I >>> think that even this would be too complicated for KASLR, which is the >>> only extremely early-boot user that I found. >>> >>> Hmm. Does the prandom stuff want to use this? >> >> prandom isn't even using rdrand. I'd suggest fixing this separately, >> or even just waiting until someone goes and deletes prandom. > > prandom is exactly the opposite; it is designed for when we need > possibly low quality random numbers very quickly. RDRAND is actually > too slow. Yep, prandom() is quite heavily used in the network stack where it's traded for speed. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/17/2014 03:33 AM, Theodore Ts'o wrote: > On Wed, Jul 16, 2014 at 09:55:15PM -0700, H. Peter Anvin wrote: >> On 07/16/2014 05:03 PM, Andy Lutomirski wrote: >>>> >>> I meant that prandom isn't using rdrand for early seeding. >>> >> >> We should probably fix that. > > It wouldn't hurt to explicitly use arch_get_random_long() in prandom, > but it does use get_random_bytes() in early seed, and for CPU's with > RDRAND present, we do use it in init_std_data() in > drivers/char/random.c, so prandom is already getting initialized via > an RNG (which is effectively a DRBG even if it doesn't pass all of > NIST's rules) which is derived from RDRAND. > I assumed he was referring to before alternatives. Not sure if we use prandom before that point, though. -hpa -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 9:39 AM, H. Peter Anvin <hpa@zytor.com> wrote: > On 07/17/2014 03:33 AM, Theodore Ts'o wrote: >> On Wed, Jul 16, 2014 at 09:55:15PM -0700, H. Peter Anvin wrote: >>> On 07/16/2014 05:03 PM, Andy Lutomirski wrote: >>>>> >>>> I meant that prandom isn't using rdrand for early seeding. >>>> >>> >>> We should probably fix that. >> >> It wouldn't hurt to explicitly use arch_get_random_long() in prandom, >> but it does use get_random_bytes() in early seed, and for CPU's with >> RDRAND present, we do use it in init_std_data() in >> drivers/char/random.c, so prandom is already getting initialized via >> an RNG (which is effectively a DRBG even if it doesn't pass all of >> NIST's rules) which is derived from RDRAND. >> > > I assumed he was referring to before alternatives. Not sure if we use > prandom before that point, though. Unless I'm reading the code wrong, the prandom_reseed_late call can happen after userspace is running. Anyway, I'm working on a near-complete rewrite of the guest part of all of this. --Andy > > -hpa > >
On Thu, Jul 17, 2014 at 10:12:27AM -0700, Andy Lutomirski wrote: > > Unless I'm reading the code wrong, the prandom_reseed_late call can > happen after userspace is running. But there is also the prandom_reseed() call, which happens early. - Ted -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 10:32 AM, Theodore Ts'o <tytso@mit.edu> wrote: > On Thu, Jul 17, 2014 at 10:12:27AM -0700, Andy Lutomirski wrote: >> >> Unless I'm reading the code wrong, the prandom_reseed_late call can >> happen after userspace is running. > > But there is also the prandom_reseed() call, which happens early. > Right -- I missed that. > - Ted
On Thu, Jul 17, 2014, at 19:34, Andy Lutomirski wrote: > On Thu, Jul 17, 2014 at 10:32 AM, Theodore Ts'o <tytso@mit.edu> wrote: > > On Thu, Jul 17, 2014 at 10:12:27AM -0700, Andy Lutomirski wrote: > >> > >> Unless I'm reading the code wrong, the prandom_reseed_late call can > >> happen after userspace is running. > > > > But there is also the prandom_reseed() call, which happens early. > > > > Right -- I missed that. prandom_init is a core_initcall, prandom_reseed is a late_initcall. During initialization of the network stack we have calls to prandom_u32 before the late_initcall happens. That said, I think it is not that important to seed prandom with rdseed/rdrand as security relevant entropy extraction should always use get_random_bytes(), but we should do it nonetheless. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 11:42 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > > On Thu, Jul 17, 2014, at 19:34, Andy Lutomirski wrote: >> On Thu, Jul 17, 2014 at 10:32 AM, Theodore Ts'o <tytso@mit.edu> wrote: >> > On Thu, Jul 17, 2014 at 10:12:27AM -0700, Andy Lutomirski wrote: >> >> >> >> Unless I'm reading the code wrong, the prandom_reseed_late call can >> >> happen after userspace is running. >> > >> > But there is also the prandom_reseed() call, which happens early. >> > >> >> Right -- I missed that. > > prandom_init is a core_initcall, prandom_reseed is a late_initcall. > During initialization of the network stack we have calls to prandom_u32 > before the late_initcall happens. That said, I think it is not that > important to seed prandom with rdseed/rdrand as security relevant > entropy extraction should always use get_random_bytes(), but we should > do it nonetheless. > Regardless, I don't want to do this as part of this patch series. One thing at a time... --Andy -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index a8f749e..4dfb539 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -593,6 +593,7 @@ config KVM_GUEST bool "KVM Guest support (including kvmclock)" depends on PARAVIRT select PARAVIRT_CLOCK + select ARCH_SLOW_RNG default y ---help--- This option enables various optimizations for running under the KVM @@ -627,6 +628,9 @@ config PARAVIRT_TIME_ACCOUNTING config PARAVIRT_CLOCK bool +config ARCH_SLOW_RNG + bool + endif #HYPERVISOR_GUEST config NO_BOOTMEM diff --git a/arch/x86/include/asm/archslowrng.h b/arch/x86/include/asm/archslowrng.h new file mode 100644 index 0000000..c8e8d0d --- /dev/null +++ b/arch/x86/include/asm/archslowrng.h @@ -0,0 +1,30 @@ +/* + * This file is part of the Linux kernel. + * + * Copyright (c) 2014 Andy Lutomirski + * Authors: Andy Lutomirski <luto@amacapital.net> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ + +#ifndef ASM_X86_ARCHSLOWRANDOM_H +#define ASM_X86_ARCHSLOWRANDOM_H + +#ifndef CONFIG_ARCH_SLOW_RNG +# error archslowrng.h should not be included if !CONFIG_ARCH_SLOW_RNG +#endif + +/* + * Performance is irrelevant here, so there's no point in using the + * paravirt ops mechanism. Instead just use a function pointer. + */ +extern int (*arch_get_slow_rng_u64)(u64 *v); + +#endif /* ASM_X86_ARCHSLOWRANDOM_H */ diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 3dd8e2c..8d64d28 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -416,6 +416,25 @@ void kvm_disable_steal_time(void) wrmsr(MSR_KVM_STEAL_TIME, 0, 0); } +static int nop_get_slow_rng_u64(u64 *v) +{ + return 0; +} + +static int kvm_get_slow_rng_u64(u64 *v) +{ + /* + * Allow migration from a hypervisor with the GET_RNG_SEED + * feature to a hypervisor without it. + */ + if (rdmsrl_safe(MSR_KVM_GET_RNG_SEED, v) == 0) + return 1; + else + return 0; +} + +int (*arch_get_slow_rng_u64)(u64 *v) = nop_get_slow_rng_u64; + #ifdef CONFIG_SMP static void __init kvm_smp_prepare_boot_cpu(void) { @@ -493,6 +512,9 @@ void __init kvm_guest_init(void) if (kvmclock_vsyscall) kvm_setup_vsyscall_timeinfo(); + if (kvm_para_has_feature(KVM_FEATURE_GET_RNG_SEED)) + arch_get_slow_rng_u64 = kvm_get_slow_rng_u64; + #ifdef CONFIG_SMP smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu; register_cpu_notifier(&kvm_cpu_notifier); diff --git a/include/linux/random.h b/include/linux/random.h index 57fbbff..ceafbcf 100644 --- a/include/linux/random.h +++ b/include/linux/random.h @@ -106,6 +106,15 @@ static inline int arch_has_random_seed(void) } #endif +#ifdef CONFIG_ARCH_SLOW_RNG +# include <asm/archslowrng.h> +#else +static inline int arch_get_slow_rng_u64(u64 *v) +{ + return 0; +} +#endif + /* Pseudo random number generator from numerical recipes. */ static inline u32 next_pseudo_random32(u32 seed) {
arch_get_slow_rng_u64 tries to get 64 bits of RNG seed data. Unlike arch_get_random_{bytes,seed}, etc., it makes no claims about entropy content. It's also likely to be much slower and should not be used frequently. That being said, it should be fast enough to call several times during boot without any noticeable slowdown. This initial implementation backs it with MSR_KVM_GET_RNG_SEED if available. The intent is for other hypervisor guest implementations to implement this interface. Signed-off-by: Andy Lutomirski <luto@amacapital.net> --- arch/x86/Kconfig | 4 ++++ arch/x86/include/asm/archslowrng.h | 30 ++++++++++++++++++++++++++++++ arch/x86/kernel/kvm.c | 22 ++++++++++++++++++++++ include/linux/random.h | 9 +++++++++ 4 files changed, 65 insertions(+) create mode 100644 arch/x86/include/asm/archslowrng.h