| Message ID | 20200110122341.8445-3-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARMv8.5-RNG support | expand |
On Fri, Jan 10, 2020 at 12:23:40PM +0000, Mark Brown wrote: > Since the arm64 ARCH_RANDOM implementation is not available until > cpufeature has determined the system capabilities it can't be used by > the generic random code to initialize the entropy pool for early use. > Instead explicitly add some data to the pool from setup_arch() if the > boot CPU supports v8.5-RNG, this is the point recommended by the generic > code. > > Note that we are only adding data here, it will be mixed into the pool > but won't be credited as entropy. There are currently no suitable > interfaces for that at present - extending the random code to provide > those will be done as a future step. Providing data is better than not > doing so as it will still provide an increase in variation in the output > from the random code and there will be no impact on the rate at which > entropy is credited compared to what we have without this patch. > > Signed-off-by: Mark Brown <broonie@kernel.org> This is certainly better than the current state of things so: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > --- > arch/arm64/include/asm/archrandom.h | 30 +++++++++++++++++++++++++++++ > arch/arm64/kernel/setup.c | 2 ++ > 2 files changed, 32 insertions(+) > > diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h > index 5ea5a1ce5a5f..2eb1db1f0bdf 100644 > --- a/arch/arm64/include/asm/archrandom.h > +++ b/arch/arm64/include/asm/archrandom.h > @@ -59,9 +59,39 @@ static inline bool __must_check arch_get_random_seed_int(unsigned int *v) > return ok; > } > > +static inline bool __init __early_cpu_has_rndr(void) > +{ > + /* Open code as we run prior to the first call to cpufeature. */ > + unsigned long ftr = read_sysreg_s(SYS_ID_AA64ISAR0_EL1); > + return (ftr >> ID_AA64ISAR0_RNDR_SHIFT) & 0xf; > +} > + > +/* > + * Our ARCH_RANDOM implementation does not function until relatively > + * late in the boot when cpufeature has detertmined system > + * capabilities so the core code can't use arch_get_random*() to > + * initialize, instead we call this function to inject data from > + * setup_arch() if the boot CPU supports v8.5-RNG. > + */ > +static inline void __init arm64_add_early_rndr_entropy(void) > +{ > + unsigned long val; > + int i; > + > + if (!__early_cpu_has_rndr()) > + return; > + > + /* Add multiple values to mirror the generic code. */ > + for (i = 0; i < 16; i++) > + if (__arm64_rndr(&val)) > + add_device_randomness(&val, sizeof(val)); > +} > + > #else > > static inline bool __arm64_rndr(unsigned long *v) { return false; } > +static inline bool __init __early_cpu_has_rndr(void) { return false; } > +static inline void __init arm64_add_early_rndr_entropy(void) { } > > #endif /* CONFIG_ARCH_RANDOM */ > #endif /* _ASM_ARCHRANDOM_H */ > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 56f664561754..170842965a32 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -344,6 +344,8 @@ void __init setup_arch(char **cmdline_p) > /* Init percpu seeds for random tags after cpus are set up. */ > kasan_init_tags(); > > + arm64_add_early_rndr_entropy(); > + > #ifdef CONFIG_ARM64_SW_TTBR0_PAN > /* > * Make sure init_thread_info.ttbr0 always generates translation > -- > 2.20.1 >
On 1/10/20 2:23 AM, Mark Brown wrote: > Since the arm64 ARCH_RANDOM implementation is not available until > cpufeature has determined the system capabilities it can't be used by > the generic random code to initialize the entropy pool for early use. > Instead explicitly add some data to the pool from setup_arch() if the > boot CPU supports v8.5-RNG, this is the point recommended by the generic > code. > > Note that we are only adding data here, it will be mixed into the pool > but won't be credited as entropy. There are currently no suitable > interfaces for that at present - extending the random code to provide > those will be done as a future step. Providing data is better than not > doing so as it will still provide an increase in variation in the output > from the random code and there will be no impact on the rate at which > entropy is credited compared to what we have without this patch. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/include/asm/archrandom.h | 30 +++++++++++++++++++++++++++++ > arch/arm64/kernel/setup.c | 2 ++ > 2 files changed, 32 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Fri, 10 Jan 2020 at 13:23, Mark Brown <broonie@kernel.org> wrote: > > Since the arm64 ARCH_RANDOM implementation is not available until > cpufeature has determined the system capabilities it can't be used by > the generic random code to initialize the entropy pool for early use. > Instead explicitly add some data to the pool from setup_arch() if the > boot CPU supports v8.5-RNG, this is the point recommended by the generic > code. > > Note that we are only adding data here, it will be mixed into the pool > but won't be credited as entropy. There are currently no suitable > interfaces for that at present - extending the random code to provide > those will be done as a future step. Providing data is better than not > doing so as it will still provide an increase in variation in the output > from the random code and there will be no impact on the rate at which > entropy is credited compared to what we have without this patch. > This is slightly unfortunate, as this way, we lose the ability to use random.trust_cpu=1 to get the entropy credited and initialize CRNG early. > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/include/asm/archrandom.h | 30 +++++++++++++++++++++++++++++ > arch/arm64/kernel/setup.c | 2 ++ > 2 files changed, 32 insertions(+) > > diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h > index 5ea5a1ce5a5f..2eb1db1f0bdf 100644 > --- a/arch/arm64/include/asm/archrandom.h > +++ b/arch/arm64/include/asm/archrandom.h > @@ -59,9 +59,39 @@ static inline bool __must_check arch_get_random_seed_int(unsigned int *v) > return ok; > } > > +static inline bool __init __early_cpu_has_rndr(void) > +{ > + /* Open code as we run prior to the first call to cpufeature. */ > + unsigned long ftr = read_sysreg_s(SYS_ID_AA64ISAR0_EL1); > + return (ftr >> ID_AA64ISAR0_RNDR_SHIFT) & 0xf; > +} > + > +/* > + * Our ARCH_RANDOM implementation does not function until relatively > + * late in the boot when cpufeature has detertmined system determined > + * capabilities so the core code can't use arch_get_random*() to > + * initialize, instead we call this function to inject data from > + * setup_arch() if the boot CPU supports v8.5-RNG. > + */ > +static inline void __init arm64_add_early_rndr_entropy(void) > +{ > + unsigned long val; > + int i; > + > + if (!__early_cpu_has_rndr()) > + return; > + > + /* Add multiple values to mirror the generic code. */ > + for (i = 0; i < 16; i++) > + if (__arm64_rndr(&val)) > + add_device_randomness(&val, sizeof(val)); > +} > + > #else > > static inline bool __arm64_rndr(unsigned long *v) { return false; } > +static inline bool __init __early_cpu_has_rndr(void) { return false; } > +static inline void __init arm64_add_early_rndr_entropy(void) { } > > #endif /* CONFIG_ARCH_RANDOM */ > #endif /* _ASM_ARCHRANDOM_H */ > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 56f664561754..170842965a32 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -344,6 +344,8 @@ void __init setup_arch(char **cmdline_p) > /* Init percpu seeds for random tags after cpus are set up. */ > kasan_init_tags(); > > + arm64_add_early_rndr_entropy(); > + > #ifdef CONFIG_ARM64_SW_TTBR0_PAN > /* > * Make sure init_thread_info.ttbr0 always generates translation > -- > 2.20.1 >
On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote: > On Fri, 10 Jan 2020 at 13:23, Mark Brown <broonie@kernel.org> wrote: > > > > Since the arm64 ARCH_RANDOM implementation is not available until > > cpufeature has determined the system capabilities it can't be used by > > the generic random code to initialize the entropy pool for early use. > > Instead explicitly add some data to the pool from setup_arch() if the > > boot CPU supports v8.5-RNG, this is the point recommended by the generic > > code. > > > > Note that we are only adding data here, it will be mixed into the pool > > but won't be credited as entropy. There are currently no suitable > > interfaces for that at present - extending the random code to provide > > those will be done as a future step. Providing data is better than not > > doing so as it will still provide an increase in variation in the output > > from the random code and there will be no impact on the rate at which > > entropy is credited compared to what we have without this patch. > > > > This is slightly unfortunate, as this way, we lose the ability to use > random.trust_cpu=1 to get the entropy credited and initialize CRNG > early. Agreed. Do you think we should wait for that support before merging the series? Given that I don't know of any CPUs implementing this extension, we can probably afford not to rush this in. Will
On Wed, 15 Jan 2020 at 10:16, Will Deacon <will@kernel.org> wrote: > > On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote: > > On Fri, 10 Jan 2020 at 13:23, Mark Brown <broonie@kernel.org> wrote: > > > > > > Since the arm64 ARCH_RANDOM implementation is not available until > > > cpufeature has determined the system capabilities it can't be used by > > > the generic random code to initialize the entropy pool for early use. > > > Instead explicitly add some data to the pool from setup_arch() if the > > > boot CPU supports v8.5-RNG, this is the point recommended by the generic > > > code. > > > > > > Note that we are only adding data here, it will be mixed into the pool > > > but won't be credited as entropy. There are currently no suitable > > > interfaces for that at present - extending the random code to provide > > > those will be done as a future step. Providing data is better than not > > > doing so as it will still provide an increase in variation in the output > > > from the random code and there will be no impact on the rate at which > > > entropy is credited compared to what we have without this patch. > > > > > > > This is slightly unfortunate, as this way, we lose the ability to use > > random.trust_cpu=1 to get the entropy credited and initialize CRNG > > early. > > Agreed. Do you think we should wait for that support before merging the > series? Given that I don't know of any CPUs implementing this extension, > we can probably afford not to rush this in. > In a previous iteration, we did have a functional arch_get_random_seed_long() early on, which would solve this issue without even needing a patch like this. Perhaps Mark (Rutland) can give a recap of his concerns at the time?
On Wed, Jan 15, 2020 at 10:22:03AM +0100, Ard Biesheuvel wrote: > On Wed, 15 Jan 2020 at 10:16, Will Deacon <will@kernel.org> wrote: > > > > On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote: > > > On Fri, 10 Jan 2020 at 13:23, Mark Brown <broonie@kernel.org> wrote: > > > > > > > > Since the arm64 ARCH_RANDOM implementation is not available until > > > > cpufeature has determined the system capabilities it can't be used by > > > > the generic random code to initialize the entropy pool for early use. > > > > Instead explicitly add some data to the pool from setup_arch() if the > > > > boot CPU supports v8.5-RNG, this is the point recommended by the generic > > > > code. > > > > > > > > Note that we are only adding data here, it will be mixed into the pool > > > > but won't be credited as entropy. There are currently no suitable > > > > interfaces for that at present - extending the random code to provide > > > > those will be done as a future step. Providing data is better than not > > > > doing so as it will still provide an increase in variation in the output > > > > from the random code and there will be no impact on the rate at which > > > > entropy is credited compared to what we have without this patch. > > > > > > > > > > This is slightly unfortunate, as this way, we lose the ability to use > > > random.trust_cpu=1 to get the entropy credited and initialize CRNG > > > early. > > > > Agreed. Do you think we should wait for that support before merging the > > series? Given that I don't know of any CPUs implementing this extension, > > we can probably afford not to rush this in. > > In a previous iteration, we did have a functional > arch_get_random_seed_long() early on, which would solve this issue > without even needing a patch like this. > > Perhaps Mark (Rutland) can give a recap of his concerns at the time? It meant that the common runtime path had code that was only ever meant to run at boot time, and would also run on secondary CPUs until we finalized the caps, so they'd behave inconsistently across boot and hotplug paths. I was concerned that this was messy and would be painful to reason about and debug. My suggestion was that we either: (a) Had the arch code explicitly inject the entropy in the primary setup path, as these patches do, or; (b) Had a new callback (e.g. __early_arch_get_random_seed_long()) that the core random code only called during its initialization, separate to the runtime paths. Thanks, Mark.
On Wed, Jan 15, 2020 at 09:16:16AM +0000, Will Deacon wrote: > On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote: > > > Note that we are only adding data here, it will be mixed into the pool > > > but won't be credited as entropy. There are currently no suitable > > > interfaces for that at present - extending the random code to provide > > This is slightly unfortunate, as this way, we lose the ability to use > > random.trust_cpu=1 to get the entropy credited and initialize CRNG > > early. Right. OTOH that's a bit of a mess to do, I do have some thoughts but it's a bit of a mess trying to do it tastefully, especially when considering that you probably don't want an interface that it's easy for something to misuse. The effort involved certainly seems large enough to handle separately. > Agreed. Do you think we should wait for that support before merging the > series? Given that I don't know of any CPUs implementing this extension, > we can probably afford not to rush this in. It's implemented in at least the fast models already, not checked any of the other emulators, so there's some possibility of people using it while developing other things and hopefully at least some of the various CI systems will be including emulated platforms with newer extensions in their coverage so might gain some benefit from it. Frankly the only reason I'm looking at this at all is that I'd written patch 3 because I was getting fed up with KASLR initialization being easily disabled when I was trying to test E0PD on the models (especially before I added the status print at boot to KASLR so this happened silently), having this in mainline would've helped considerably when working on that. I don't see any downside to having the code in mainline as is, even though it's not ideal it does make things better since if for some reason anyone does end up running this code on a system that has the feature they'll get at least some benefit from it even if nothing else happens. The bulk of the code isn't going to change when the early init stuff gets improved and includes tables like cpufeature.h that make it annoying to hold out of tree, the bits that are going to change can just as well be worked on incrementally as held out of tree entirely and having the rest in means there's less friction doing that.
On Wed, Jan 15, 2020 at 12:07:03PM +0000, Mark Brown wrote: > On Wed, Jan 15, 2020 at 09:16:16AM +0000, Will Deacon wrote: > > On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote: > > > > > Note that we are only adding data here, it will be mixed into the pool > > > > but won't be credited as entropy. There are currently no suitable > > > > interfaces for that at present - extending the random code to provide > > > > This is slightly unfortunate, as this way, we lose the ability to use > > > random.trust_cpu=1 to get the entropy credited and initialize CRNG > > > early. > > Right. OTOH that's a bit of a mess to do, I do have some > thoughts but it's a bit of a mess trying to do it tastefully, > especially when considering that you probably don't want an > interface that it's easy for something to misuse. The effort > involved certainly seems large enough to handle separately. Maybe, but see below... > > Agreed. Do you think we should wait for that support before merging the > > series? Given that I don't know of any CPUs implementing this extension, > > we can probably afford not to rush this in. > > It's implemented in at least the fast models already, not checked > any of the other emulators, so there's some possibility of people > using it while developing other things and hopefully at least > some of the various CI systems will be including emulated > platforms with newer extensions in their coverage so might gain > some benefit from it. Frankly the only reason I'm looking at > this at all is that I'd written patch 3 because I was getting fed > up with KASLR initialization being easily disabled when I was > trying to test E0PD on the models (especially before I added the > status print at boot to KASLR so this happened silently), having > this in mainline would've helped considerably when working on > that. I was thinking specifically about users on silicon rather than developers on simulators. (I could stick this on a branch for developers if necessary). > I don't see any downside to having the code in mainline as is, > even though it's not ideal it does make things better since if > for some reason anyone does end up running this code on a system > that has the feature they'll get at least some benefit from it > even if nothing else happens. The bulk of the code isn't going > to change when the early init stuff gets improved and includes > tables like cpufeature.h that make it annoying to hold out of > tree, the bits that are going to change can just as well be > worked on incrementally as held out of tree entirely and having > the rest in means there's less friction doing that. The usual downside that comes from merging patches with promises of fixing them up later is that the motivating task gets marked as "done" somewhere, the developer gets given something else to do and the updates never materialise. That's not a dig at you; it's just the way these things tend to work (I've certainly been on both sides of that coin). If there was an urgency to this, I'd suggest merging a form of Richard's code, as it appears to solve the technical issue of credited entropy whilst leaving some room for subsequent cleanup. However, I think that makes it even less likely that anybody will come back to do the cleanup because the code will be perfectly functional, so I'd prefer to wait for a complete solution unless you think it's not achievable for 5.7. I'd also really like Ard's ack on anything relating to RNGs. Will
On Wed, 15 Jan 2020 at 13:42, Will Deacon <will@kernel.org> wrote: > > On Wed, Jan 15, 2020 at 12:07:03PM +0000, Mark Brown wrote: > > On Wed, Jan 15, 2020 at 09:16:16AM +0000, Will Deacon wrote: > > > On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote: > > > > > > > Note that we are only adding data here, it will be mixed into the pool > > > > > but won't be credited as entropy. There are currently no suitable > > > > > interfaces for that at present - extending the random code to provide > > > > > > This is slightly unfortunate, as this way, we lose the ability to use > > > > random.trust_cpu=1 to get the entropy credited and initialize CRNG > > > > early. > > > > Right. OTOH that's a bit of a mess to do, I do have some > > thoughts but it's a bit of a mess trying to do it tastefully, > > especially when considering that you probably don't want an > > interface that it's easy for something to misuse. The effort > > involved certainly seems large enough to handle separately. > > Maybe, but see below... > > > > Agreed. Do you think we should wait for that support before merging the > > > series? Given that I don't know of any CPUs implementing this extension, > > > we can probably afford not to rush this in. > > > > It's implemented in at least the fast models already, not checked > > any of the other emulators, so there's some possibility of people > > using it while developing other things and hopefully at least > > some of the various CI systems will be including emulated > > platforms with newer extensions in their coverage so might gain > > some benefit from it. Frankly the only reason I'm looking at > > this at all is that I'd written patch 3 because I was getting fed > > up with KASLR initialization being easily disabled when I was > > trying to test E0PD on the models (especially before I added the > > status print at boot to KASLR so this happened silently), having > > this in mainline would've helped considerably when working on > > that. > > I was thinking specifically about users on silicon rather than developers > on simulators. (I could stick this on a branch for developers if necessary). > > > I don't see any downside to having the code in mainline as is, > > even though it's not ideal it does make things better since if > > for some reason anyone does end up running this code on a system > > that has the feature they'll get at least some benefit from it > > even if nothing else happens. The bulk of the code isn't going > > to change when the early init stuff gets improved and includes > > tables like cpufeature.h that make it annoying to hold out of > > tree, the bits that are going to change can just as well be > > worked on incrementally as held out of tree entirely and having > > the rest in means there's less friction doing that. > > The usual downside that comes from merging patches with promises of fixing > them up later is that the motivating task gets marked as "done" somewhere, > the developer gets given something else to do and the updates never > materialise. That's not a dig at you; it's just the way these things tend > to work (I've certainly been on both sides of that coin). > > If there was an urgency to this, I'd suggest merging a form of Richard's > code, as it appears to solve the technical issue of credited entropy whilst > leaving some room for subsequent cleanup. However, I think that makes it > even less likely that anybody will come back to do the cleanup because the > code will be perfectly functional, so I'd prefer to wait for a complete > solution unless you think it's not achievable for 5.7. > > I'd also really like Ard's ack on anything relating to RNGs. > Patches #1 and #3 are fine with me, modulo the HWCAP bit which I don't deeply care about. But the way this patch works around our workaround for mismatched RNG caps between cores doesn't make sense to me. arch_get_random_seed_long() should just have some out of line __init path that gets invoked only during early boot, exactly how we are using it in patch #3 to seed KASLR, where we don't care about whether or not other CPUs have the extension. (Note that rand_initialize() is called very early, way before the point where we have to care about being scheduled from a CPU with RNG to one without)
On Wed, Jan 15, 2020 at 10:11:08AM +0000, Mark Rutland wrote: > On Wed, Jan 15, 2020 at 10:22:03AM +0100, Ard Biesheuvel wrote: > > In a previous iteration, we did have a functional > > arch_get_random_seed_long() early on, which would solve this issue > > without even needing a patch like this. > It meant that the common runtime path had code that was only ever meant > to run at boot time, and would also run on secondary CPUs until we > finalized the caps, so they'd behave inconsistently across boot and > hotplug paths. I was concerned that this was messy and would be painful > to reason about and debug. > My suggestion was that we either: > (a) Had the arch code explicitly inject the entropy in the primary setup > path, as these patches do, or; These patches don't quite do that, they inject data but not entropy so anything that is waiting for the pool to become fully initialized will still end up waiting, though we do still get the data mixed in. There is currently no interface which allows one to explicitly inject entropy as though from the architecture and I'm not convinced that having one would be a good idea. > (b) Had a new callback (e.g. __early_arch_get_random_seed_long()) that > the core random code only called during its initialization, separate > to the runtime paths. This is definitely an option, but it is a bit ugly and as things stand with random.c it would I think have to cope with possibly running with multiple processors at which point we start to get back to the complexity you were originally worried about just in a code path that's less commonly executed.
On Wed, Jan 15, 2020 at 12:42:39PM +0000, Will Deacon wrote: > On Wed, Jan 15, 2020 at 12:07:03PM +0000, Mark Brown wrote: > > tables like cpufeature.h that make it annoying to hold out of > > tree, the bits that are going to change can just as well be > > worked on incrementally as held out of tree entirely and having > > the rest in means there's less friction doing that. > The usual downside that comes from merging patches with promises of fixing > them up later is that the motivating task gets marked as "done" somewhere, > the developer gets given something else to do and the updates never > materialise. That's not a dig at you; it's just the way these things tend > to work (I've certainly been on both sides of that coin). It's certainly a way things can work, but it does assume that there is an underlying motivating task that involves getting things upstream which will cause people to do the additional work rather than just wandering off and then potentially causing someone else to redo the bit that was already done if they don't notice the code in the list archives or spend time trying to figure out if the original author will continue revising their series. We even had an awkward situation when I was at Linaro where the original author of something we depended on was continuing to work on their series but it was now a spare time activity for them so progress was painfully slow, the worst of both worlds. The most common way this happens that I run into is people implementing things for products who are doing the upstreaming on the side. It can also have the effect of discouraging people from trying to do things in the first place, I know the likelyhood of scope creep is one of the factors that influences how likely I am to try to improve things I happen to notice while I'm working on something else and I'm fairly sure other people make similar assessments. > If there was an urgency to this, I'd suggest merging a form of Richard's > code, as it appears to solve the technical issue of credited entropy whilst > leaving some room for subsequent cleanup. However, I think that makes it > even less likely that anybody will come back to do the cleanup because the I agree with your assessment of the likelyhood of cleanup, I think an incomplete solution that doesn't credit entropy is more robustly likely to get fixed since it causes an actual problem that people will be motivated to fix as opposed to just being ugly code. I have no objection to merging Richard's static_branch_likely() approach though. > code will be perfectly functional, so I'd prefer to wait for a complete > solution unless you think it's not achievable for 5.7. It could happen but I wouldn't like to commit to getting something in for v5.7, that's basically just a single release given how near we are to the v5.6 merge window opening and I know changes in the random code can sometimes take a while.
On Wed, Jan 15, 2020 at 02:36:32PM +0100, Ard Biesheuvel wrote: > On Wed, 15 Jan 2020 at 13:42, Will Deacon <will@kernel.org> wrote: > > I'd also really like Ard's ack on anything relating to RNGs. > Patches #1 and #3 are fine with me, modulo the HWCAP bit which I don't > deeply care about. > But the way this patch works around our workaround for mismatched RNG > caps between cores doesn't make sense to me. I'd be totally happy to drop patch 2 entirely, it's a *bit* marginal if it's useful - I mainly wrote it because it's so trivial to do not because I think it's a wonderful idea. > arch_get_random_seed_long() should just have some out of line __init > path that gets invoked only during early boot, exactly how we are > using it in patch #3 to seed KASLR, where we don't care about whether Yes, I think that would be a good place to get to if we can - if the early init thing is a separate call then we have to worry about the callers always running from the right context which sounds like trouble. It's just trying to figure out a way to write things which is clearly robust when looking at the arch code by itself, and I don't want to completely discount the possibility of a new interface from the random code to help with that yet. > or not other CPUs have the extension. (Note that rand_initialize() is > called very early, way before the point where we have to care about > being scheduled from a CPU with RNG to one without) Everything is simple during rand_initialize(), though the actual calls to get entropy that it does happen in crng_initialize() which is also used to initialize the per-node pools for NUMA systems but that should happen after the capabilities code has run I think (pretty sure, but I need to check) so we can rely on cpus_have_const_cap(). There's also calls in prandom_init() which runs at core_initcall() so that should be fine too.
On Wed, Jan 15, 2020 at 05:04:59PM +0000, Mark Brown wrote: > On Wed, Jan 15, 2020 at 02:36:32PM +0100, Ard Biesheuvel wrote: > > On Wed, 15 Jan 2020 at 13:42, Will Deacon <will@kernel.org> wrote: > > > > I'd also really like Ard's ack on anything relating to RNGs. > > > Patches #1 and #3 are fine with me, modulo the HWCAP bit which I don't > > deeply care about. > > > But the way this patch works around our workaround for mismatched RNG > > caps between cores doesn't make sense to me. > > I'd be totally happy to drop patch 2 entirely, it's a *bit* > marginal if it's useful - I mainly wrote it because it's so > trivial to do not because I think it's a wonderful idea. OK, tell you what -- please resend just 1 and 3, with the HWCAP addition (because we're not going to resolve that in time for 5.6) and I'll queue them up. Will
diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h index 5ea5a1ce5a5f..2eb1db1f0bdf 100644 --- a/arch/arm64/include/asm/archrandom.h +++ b/arch/arm64/include/asm/archrandom.h @@ -59,9 +59,39 @@ static inline bool __must_check arch_get_random_seed_int(unsigned int *v) return ok; } +static inline bool __init __early_cpu_has_rndr(void) +{ + /* Open code as we run prior to the first call to cpufeature. */ + unsigned long ftr = read_sysreg_s(SYS_ID_AA64ISAR0_EL1); + return (ftr >> ID_AA64ISAR0_RNDR_SHIFT) & 0xf; +} + +/* + * Our ARCH_RANDOM implementation does not function until relatively + * late in the boot when cpufeature has detertmined system + * capabilities so the core code can't use arch_get_random*() to + * initialize, instead we call this function to inject data from + * setup_arch() if the boot CPU supports v8.5-RNG. + */ +static inline void __init arm64_add_early_rndr_entropy(void) +{ + unsigned long val; + int i; + + if (!__early_cpu_has_rndr()) + return; + + /* Add multiple values to mirror the generic code. */ + for (i = 0; i < 16; i++) + if (__arm64_rndr(&val)) + add_device_randomness(&val, sizeof(val)); +} + #else static inline bool __arm64_rndr(unsigned long *v) { return false; } +static inline bool __init __early_cpu_has_rndr(void) { return false; } +static inline void __init arm64_add_early_rndr_entropy(void) { } #endif /* CONFIG_ARCH_RANDOM */ #endif /* _ASM_ARCHRANDOM_H */ diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 56f664561754..170842965a32 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -344,6 +344,8 @@ void __init setup_arch(char **cmdline_p) /* Init percpu seeds for random tags after cpus are set up. */ kasan_init_tags(); + arm64_add_early_rndr_entropy(); + #ifdef CONFIG_ARM64_SW_TTBR0_PAN /* * Make sure init_thread_info.ttbr0 always generates translation
Since the arm64 ARCH_RANDOM implementation is not available until cpufeature has determined the system capabilities it can't be used by the generic random code to initialize the entropy pool for early use. Instead explicitly add some data to the pool from setup_arch() if the boot CPU supports v8.5-RNG, this is the point recommended by the generic code. Note that we are only adding data here, it will be mixed into the pool but won't be credited as entropy. There are currently no suitable interfaces for that at present - extending the random code to provide those will be done as a future step. Providing data is better than not doing so as it will still provide an increase in variation in the output from the random code and there will be no impact on the rate at which entropy is credited compared to what we have without this patch. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/archrandom.h | 30 +++++++++++++++++++++++++++++ arch/arm64/kernel/setup.c | 2 ++ 2 files changed, 32 insertions(+)