Re: get_random_bytes returns bad randomness before seeding is complete

[Jason A. Donenfeld]

On Sat, Jun 3, 2017 at 7:04 AM, Theodore Ts'o  wrote:
> has been pretty terrible?
> This kind of "my shit doesn't stink, but yours does", is not
> The reason why I keep harping on this is because I'm concerned about
> an absolutist attitude towards technical design, where the good is the

Moving past that, did you see the [PATCH RCF 0/3] series I posted
yesterday? Would be helpful to have your feedback on that approach and
implementation strategy. Since it seems like you're preferring
cleaning up things individually, rather than the systemic rnginit
solution I initially proposed, I moved forward with implementing an
RFC-version of that. I'm pretty sure so quickly compromising and going
with what I perceived you thought was best is a strong indication that
there isn't an, "absolutist attitude towards technical design".
However, if you do somehow find evidence of that kind of claim in my
[PATCH] set, please do bring it up, and I'll try to adjust to be more
pleasing.

> We're going to have to look at a representative sample of the call
> sites to figure this out.  The simple case is where the call site is
> only run in response to a userspace system call.  There, blocking
> makes perfect sense.  I'm just not sure there are many callers of
> get_random_ bytes() where this is the case.

In the patch series I sent earlier, the reason I split things into
wait_for_random_bytes, which just blocks until the pool is ready, and
then the convenience combiner of get_random_bytes_wait, which calls
wait_for_random_bytes and then get_random_bytes, is because I was
thinking there might be a few places where we can't actually sleep
during the get_random_bytes call, due to in_interrupt() or whatever,
but that there's some process-context area that's _always_ called
before get_random_bytes, like a userspace configuration API or an
ioctl, so we could simply put a call to wait_for_random_bytes, and
then be sure that all calls to get_random_bytes after that are safe.

I guess I'll see in practice if this is actually a useful way of doing
it, once I dig in and start modifying representative call sites.

> When would a timeout be useful?  If you are using get_random_bytes()
> for security reasons, does the security reason go away after 15
> seconds?  Or even 30 seconds?

I was thinking that returning to userspace with -ETIMEDOUT or
something might be more desirable in some odd situations (which ones?)
than just waiting for a signal and responding with
-EINTR/-ERESTARTSYS.  That might turn out to be not true, in which
case I guess I won't add that API, as you suggested.

> Also, it is possible that we may have architectures, without
> fine-grained clocks, where we don't initialize the rng until after
> userspace as sharted running.  So it's not clear adding a rnginit
> section makes sense.  Even if we put it as late as possible --- say,
> after "late", what do we do if don't have the CRNG fully
> negotiated after the last of the "late" drivers have been run?

My idea was that it would be eventually inserted on the callback from
add_random_ready_callback. You're right that this would not be okay
for things like filesystems, but maybe it'd be appropriate for things
like crypto/rng.c? Or, perhaps the blocking API on configuration-time
would be better, anyway, for things like that. You seem wary of this
approach, so I'm going to roll with your suggestions above and see how
they work out. It it pans out great, if not, maybe we'll revisit this
down the road once I have a better picture of what the call sites are
like.

Jason

Re: [PATCH V2 00/27] Drivers: ccree - align block comments

[Greg KH]

On Tue, May 30, 2017 at 06:09:37PM +1200, Derek Robson wrote:
> Fixed block comments across whole ccree driver
> 
> Version #1 has some trailing white space issue in a few patches

Some of these applied, some did not :(

Re: [PATCH V2 01/27] Drivers: ccree: ssi_sysfs.h - align block comments

[Greg KH]

On Tue, May 30, 2017 at 06:09:54PM +1200, Derek Robson wrote:
> Fixed block comment alignment, Style fix only
> Found using checkpatch
> 
> Signed-off-by: Derek Robson 
> ---
>  drivers/staging/ccree/ssi_sysfs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Does not apply to my tree :(

Re: [PATCH] Staging: ccree: cc_crypto_ctx.h: Fixed a comment coding style issue.

[Greg KH]

On Wed, May 31, 2017 at 06:24:55PM +0530, srishti sharma wrote:
> Fixed a comment coding style issue that generated a warning stating that 
> block comments should align the * on each line.
> 

Please properly line-wrap your changelog comments :(

You also sent 2 different patches, that did different things, yet they
had the same subject line.  That's not ok, and also, I have no idea
which one to apply first.

I'm dropping both, please fix and resend them properly as a patch
series.

thanks,

greg k-h

Re: [PATCH] Staging: ccree: ssi_aead.h: Fixed a pointer declaration error.

[Greg KH]

On Sat, Jun 03, 2017 at 04:15:17AM +0530, srishti sharma wrote:
> Fixed a pointer declaration error , the dereferencing operator was misplaced.

Odd use of a ,

Also, I don't understand what was "misplaced" here?  There does not seem
to be a "error" fixed here...

Re: [PATCH v2 02/20] staging: ccree: replace bit shift with BIT macro

[Greg Kroah-Hartman]

On Thu, Jun 01, 2017 at 02:02:52PM +0300, Gilad Ben-Yossef wrote:
> CC_CTX_SIZE was being defined using a hand rolled bit shift operation.
> Replace with use of BIT macro.
> 
> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/staging/ccree/cc_crypto_ctx.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This patch didn't apply at all, are you sure you are working against the
correct tree?  I've tried to apply some of this series, but most do not
work at all...

Please rebase against my staging-next branch and resend the ones that do
work.

thanks,

greg k-h

[PATCH] Staging: ccree: ssi_aead.h: Fixed a comment coding style issue.

[srishti sharma]

Fixed a comment coding style issue , block comments use * on subsequent lines.

Signed-off-by: srishti sharma 
---
 drivers/staging/ccree/ssi_aead.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ccree/ssi_aead.h b/drivers/staging/ccree/ssi_aead.h
index 9cf5225..4b592b7 100644
--- a/drivers/staging/ccree/ssi_aead.h
+++ b/drivers/staging/ccree/ssi_aead.h
@@ -14,8 +14,9 @@
  * along with this program; if not, see .
  */

-/* \file ssi_aead.h
-   ARM CryptoCell AEAD Crypto API
+/*
+ * \file ssi_aead.h
+ *  ARM CryptoCell AEAD Crypto API
  */

 #ifndef __SSI_AEAD_H__
--
2.7.4

Re: [PATCH] Staging: ccree: ssi_aead.h: Fixed a pointer declaration error.

[srishti sharma]

Hey,

checkpatch.pl generated two errors , because the dereferencing
operator was placed next to the structure name instead of being placed
with the pointer .

for eg:
  struct scatterlist* srcSgl; (this was giving an error)

whereas
 struct scatterlist *srcSgl; (this did not give an error)


Both of them will compile , but the second one is a better
representation of it and does not produce an error on running
checkpatch.pl .

Regards,
Srishti

On Sat, Jun 3, 2017 at 2:07 PM, Greg KH  wrote:
> On Sat, Jun 03, 2017 at 04:15:17AM +0530, srishti sharma wrote:
>> Fixed a pointer declaration error , the dereferencing operator was misplaced.
>
> Odd use of a ,
>
> Also, I don't understand what was "misplaced" here?  There does not seem
> to be a "error" fixed here...
>

Re: get_random_bytes returns bad randomness before seeding is complete

[Sandy Harris]

Stephan's driver, the HAVEGE system & several others purport to
extract entropy from a series of timer calls. Probably the best
analysis is in the Mcguire et al. paper at
https://static.lwn.net/images/conf/rtlws11/random-hardware.pdf & the
simplest code in my user-space driver at
https://github.com/sandy-harris/maxwell The only kernel-space code I
know of is Stephan's.

If the claim that such calls give entropy is accepted (which I think
it should be) then if we get one bit per call, need 100 or so bits &
space the calls 100 ns apart, loading up a decent chunk of startup
entropy takes about 10,000 ns or 10 microseconds which looks like an
acceptable delay. Can we just do that very early in the boot process?

Of course this will fail on systems with no high-res timer. Are there
still some of those? It might be done in about 1000 times as long on a
system that lacks the realtime library's nanosecond timer but has the
Posix standard microsecond timer, implying a delay time in the
milliseconds. Would that be acceptable in those cases?

[PATCH][V2] staging: ccree: fix spelling mistake: "chanined" -> "chained"

[Colin King]

From: Colin Ian King 

Trivial fix to spelling mistake in SSI_LOG_ERR message

Signed-off-by: Colin Ian King 
---
 drivers/staging/ccree/ssi_buffer_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
b/drivers/staging/ccree/ssi_buffer_mgr.c
index 04515e70d2d3..379eecf85ba1 100644
--- a/drivers/staging/ccree/ssi_buffer_mgr.c
+++ b/drivers/staging/ccree/ssi_buffer_mgr.c
@@ -156,7 +156,7 @@ static unsigned int ssi_buffer_mgr_get_sgl_nents(
unsigned int nents = 0;
while (nbytes != 0) {
if (sg_is_chain(sg_list)) {
-   SSI_LOG_ERR("Unexpected chanined entry "
+   SSI_LOG_ERR("Unexpected chained entry "
   "in sg (entry =0x%X) \n", nents);
BUG();
}
-- 
2.11.0

[PATCH 0/4] Fixed comment coding style issues.

[srishti sharma]

This patchset contains a series of comment coding style issues fixes.

srishti sharma (4):
  Staging: ccree: cc_crypto_ctx.h: Added * on subsequent lines of a
comment block.
  Staging: ccree: cc_crypto_ctx.h: Fixed trailing */ issue in a comment
block
  Staging: ccree: cc_crypto_ctx.h: Fixed * alignment issues in a comment
block
  Staging: ccree: cc_crypto_ctx.h: Added * on subsequent lines of a
comment block

 drivers/staging/ccree/cc_crypto_ctx.h | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH] taging: ccree: fix spelling mistake: "chanined" -> "chained"

[Colin King]

From: Colin Ian King 

Trivial fix to spelling mistake in SSI_LOG_ERR message

Signed-off-by: Colin Ian King 
---
 drivers/staging/ccree/ssi_buffer_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
b/drivers/staging/ccree/ssi_buffer_mgr.c
index 04515e70d2d3..379eecf85ba1 100644
--- a/drivers/staging/ccree/ssi_buffer_mgr.c
+++ b/drivers/staging/ccree/ssi_buffer_mgr.c
@@ -156,7 +156,7 @@ static unsigned int ssi_buffer_mgr_get_sgl_nents(
unsigned int nents = 0;
while (nbytes != 0) {
if (sg_is_chain(sg_list)) {
-   SSI_LOG_ERR("Unexpected chanined entry "
+   SSI_LOG_ERR("Unexpected chained entry "
   "in sg (entry =0x%X) \n", nents);
BUG();
}
-- 
2.11.0

[PATCH 1/4] Staging: ccree: cc_crypto_ctx.h: Added * on subsequent lines of a comment block.

[srishti sharma]

Added * on subsequent lines of a comment block.

Signed-off-by: srishti sharma 
---
 drivers/staging/ccree/cc_crypto_ctx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ccree/cc_crypto_ctx.h 
b/drivers/staging/ccree/cc_crypto_ctx.h
index ac39d34..6ee51b8 100644
--- a/drivers/staging/ccree/cc_crypto_ctx.h
+++ b/drivers/staging/ccree/cc_crypto_ctx.h
@@ -219,7 +219,7 @@ struct drv_ctx_hash {
 };

 /*  drv_ctx_hmac should have the same structure as drv_ctx_hash except
-   k0, k0_size fields */
+ * k0, k0_size fields */
 struct drv_ctx_hmac {
enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_HMAC */
enum drv_hash_mode mode;
--
2.7.4

Re: [kernel-hardening] Re: get_random_bytes returns bad randomness before seeding is complete

[Daniel Micay]

On 3 June 2017 at 18:54, Jeffrey Walton  wrote:
> On Sat, Jun 3, 2017 at 5:45 PM, Sandy Harris  wrote:
>> ...
>> Of course this will fail on systems with no high-res timer. Are there
>> still some of those? It might be done in about 1000 times as long on a
>> system that lacks the realtime library's nanosecond timer but has the
>> Posix standard microsecond timer, implying a delay time in the
>> milliseconds. Would that be acceptable in those cases?
>
> A significant portion of the use cases should include mobile devices.
> Device sales outnumbered desktop and server sales several years ago.
>
> Many devices are sensor rich. Even the low-end ones come with
> accelorometers for gaming. A typical one has 3 or 4 sensors, and
> higher-end ones have 7 or 8 sensors. An Evo 4G has 7 of them.
>
> There's no wanting for entropy in many of the use cases. The thing
> that is lacking seems to be taking advantage of it.
>
> Jeff

Hardware random number generator support is also standard on even
low-end mobile devices. The Linux kernel now knows to feed some of the
entropy from those hardware random generators into the kernel CSPRNG
when the driver is initialized but that doesn't happen until fairly
late in the kernel's boot process. The sensors present the same issue.
They aren't available when the kernel starts needing entropy for
features like SSP and KASLR or other early boot uses, unlike
RDRAND/RDSEED on modern x86_64 CPUs.

For userspace, Android's init system blocks until a certain amount of
entropy is obtained from one for the kernel CSPRNG. It's possible for
there to be no hwrandom but I think that's very rare now since the
standard SoCs used everywhere have it available. The device vendor
would probably need to go out of the way to break it. Android also
regularly saves a persistent random seed and restores it on boot. It
also mixes in entropy from the hardware generator regularly since the
kernel didn't know how to do that before, just like it didn't know how
to grab any initial entropy from the hardware generator.

I don't think it's worth worrying too much about mobile. Slimmer
embedded devices that probably don't even save / restore a seed in
many cases or generate keys on first boot before that helps are the
real issue. At least if you're not focused on KASLR and other early
probabilistic kernel exploit mitigations where there's a lack of a way
to get entropy in early boot right now unless the bootloader helps.

[PATCH 3/4] Staging: ccree: cc_crypto_ctx.h: Fixed * alignment issues in a comment block

[srishti sharma]

Fixed the alignment of * in a comment block.

Signed-off-by: srishti sharma 
---
 drivers/staging/ccree/cc_crypto_ctx.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ccree/cc_crypto_ctx.h 
b/drivers/staging/ccree/cc_crypto_ctx.h
index 27a5914..03164624 100644
--- a/drivers/staging/ccree/cc_crypto_ctx.h
+++ b/drivers/staging/ccree/cc_crypto_ctx.h
@@ -242,9 +242,9 @@ struct drv_ctx_cipher {
u32 key_size; /* numeric value in bytes */
u32 data_unit_size; /* required for XTS */
/* block_state is the AES engine block state.
-   *  It is used by the host to pass IV or counter at initialization.
-   *  It is used by SeP for intermediate block chaining state and for
-   *  returning MAC algorithms results. */
+* It is used by the host to pass IV or counter at initialization.
+* It is used by SeP for intermediate block chaining state and for
+* returning MAC algorithms results. */
u8 block_state[CC_AES_BLOCK_SIZE];
u8 key[CC_AES_KEY_SIZE_MAX];
u8 xex_key[CC_AES_KEY_SIZE_MAX];
--
2.7.4

[PATCH 2/4] Staging: ccree: cc_crypto_ctx.h: Fixed trailing */ issue in a comment block

[srishti sharma]

Fixed trailing */ style issue in a block comment.

Signed-off-by: srishti sharma 
---
 drivers/staging/ccree/cc_crypto_ctx.h | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ccree/cc_crypto_ctx.h 
b/drivers/staging/ccree/cc_crypto_ctx.h
index 6ee51b8..27a5914 100644
--- a/drivers/staging/ccree/cc_crypto_ctx.h
+++ b/drivers/staging/ccree/cc_crypto_ctx.h
@@ -218,8 +218,10 @@ struct drv_ctx_hash {
CC_DIGEST_SIZE_MAX];
 };

-/*  drv_ctx_hmac should have the same structure as drv_ctx_hash except
- * k0, k0_size fields */
+/*
+ *  drv_ctx_hmac should have the same structure as drv_ctx_hash except
+ * k0, k0_size fields
+ */
 struct drv_ctx_hmac {
enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_HMAC */
enum drv_hash_mode mode;
@@ -285,4 +287,3 @@ struct drv_ctx_aead {
 #define GET_CTX_FIELD_ADDR(ctx, type, member) (ctx + offsetof(type, member))

 #endif /* _CC_CRYPTO_CTX_H_ */
-
--
2.7.4

[PATCH 4/4] Staging: ccree: cc_crypto_ctx.h: Added * on subsequent lines of a comment block

[srishti sharma]

Added *'s on subsequent lines of a comment block to fix coding style issues.

Signed-off-by: srishti sharma 
---
 drivers/staging/ccree/cc_crypto_ctx.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ccree/cc_crypto_ctx.h 
b/drivers/staging/ccree/cc_crypto_ctx.h
index 03164624..a8747d5 100644
--- a/drivers/staging/ccree/cc_crypto_ctx.h
+++ b/drivers/staging/ccree/cc_crypto_ctx.h
@@ -281,9 +281,9 @@ struct drv_ctx_aead {


 /* Get the address of a @member within a given @ctx address
-   @ctx: The context address
-   @type: Type of context structure
-   @member: Associated context field */
+ * @ctx: The context address
+ * @type: Type of context structure
+ * @member: Associated context field */
 #define GET_CTX_FIELD_ADDR(ctx, type, member) (ctx + offsetof(type, member))

 #endif /* _CC_CRYPTO_CTX_H_ */
--
2.7.4

Re: get_random_bytes returns bad randomness before seeding is complete

[Stephan Müller]

Am Freitag, 2. Juni 2017, 16:59:56 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> 
> Alternatively, I'm open to other solutions people might come up with.

How about stirring in some data from the Jitter RNG that we have in the kernel 
already and that is used for the DRBG in case get_random_bytes has 
insufficient entropy? Yes, two kernel developers said that this RNG is 
useless, where in fact a lot of hardware and even crypto folks say that this 
approach has merits.

In any case, it cannot destroy the (not present) entropy at boot time anyway. 
Thus, take some 32, 48 or 64 bytes from it right at the start of the kernel, 
and we should be better (from the view point of quite some folks) or not worse 
off (view point of two developers here).

As this RNG does not depend on any in-kernel facility, it is always available 
at any time.

PS: I could revive a patch adding this to random.c that I sent long ago if 
desired.

Ciao
Stephan

Re: [PATCH v2 02/20] staging: ccree: replace bit shift with BIT macro

[Gilad Ben-Yossef]

On Sat, Jun 3, 2017 at 11:46 AM, Greg Kroah-Hartman
 wrote:
> On Thu, Jun 01, 2017 at 02:02:52PM +0300, Gilad Ben-Yossef wrote:
>> CC_CTX_SIZE was being defined using a hand rolled bit shift operation.
>> Replace with use of BIT macro.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>> ---
>>  drivers/staging/ccree/cc_crypto_ctx.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> This patch didn't apply at all, are you sure you are working against the
> correct tree?  I've tried to apply some of this series, but most do not
> work at all...
>

Yes, I did. The patch series was created against your staging/staging-next
at commit ca9280d1f82a7a0165a683dc09f182329ebec352

What I believe might have happened is that you took in Gennadii Altukhov's
and Derek Robson's patch set and mine indeed no longer applies on top of theirs.

> Please rebase against my staging-next branch and resend the ones that do
> work.

Sure, I'll rebase on top of current staging/staging-next and resend
the remaining
patches

Thanks,
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: get_random_bytes returns bad randomness before seeding is complete

[Jeffrey Walton]

On Sun, Jun 4, 2017 at 1:48 AM, Stephan Müller  wrote:
> Am Freitag, 2. Juni 2017, 16:59:56 CEST schrieb Jason A. Donenfeld:
>
>> Alternatively, I'm open to other solutions people might come up with.
>
> How about stirring in some data from the Jitter RNG that we have in the kernel
> already and that is used for the DRBG in case get_random_bytes has
> insufficient entropy? Yes, two kernel developers said that this RNG is
> useless, where in fact a lot of hardware and even crypto folks say that this
> approach has merits.

Almost anything has to be better than (1) silent failures, and (2)
draining the little entropy available when the generators are starting
and trying to become operational.

The [negative] use case for (2) is systemd. See, for example,
https://github.com/systemd/systemd/issues/4167.

Jeff

Re: get_random_bytes returns bad randomness before seeding is complete

[Stephan Müller]

Am Sonntag, 4. Juni 2017, 00:54:39 CEST schrieb Jeffrey Walton:

Hi Jeffrey,

> On Sat, Jun 3, 2017 at 5:45 PM, Sandy Harris  wrote:
> > ...
> > Of course this will fail on systems with no high-res timer. Are there
> > still some of those? It might be done in about 1000 times as long on a
> > system that lacks the realtime library's nanosecond timer but has the
> > Posix standard microsecond timer, implying a delay time in the
> > milliseconds. Would that be acceptable in those cases?
> 
> A significant portion of the use cases should include mobile devices.
> Device sales outnumbered desktop and server sales several years ago.
> 
> Many devices are sensor rich. Even the low-end ones come with
> accelorometers for gaming. A typical one has 3 or 4 sensors, and
> higher-end ones have 7 or 8 sensors. An Evo 4G has 7 of them.
> 

I think those devices are covered with the kernels 4.8+. That kernel uses 
solely interrupts as noise source for the first stage we talk about here.

Not having done any particular measurements with the latest kernels on mobile 
devices, but based on my experience with my LRNG assessment, I could fathom 
that mobile devices have a fully seeded ChaCha20 DRNG before user space 
starts.

Just to give an illustration: I have a Lenovo T540 which receives more than 
256 interrupts before late_initcall. On all system with a high-res timer, each 
interrupt will give more than one bit of entropy. Conversely, on my MacBook 
Pro 2015,  at late_initcall the kernel received less than 100 interrupts. In a 
KVM guest with very little devices, I also have some 100 interrupts before 
late_initcall. These measurements are taken with the same kernel and same 
kernel configs.

Ciao
Stephan

Re: get_random_bytes returns bad randomness before seeding is complete

[Jeffrey Walton]

On Sat, Jun 3, 2017 at 5:45 PM, Sandy Harris  wrote:
> ...
> Of course this will fail on systems with no high-res timer. Are there
> still some of those? It might be done in about 1000 times as long on a
> system that lacks the realtime library's nanosecond timer but has the
> Posix standard microsecond timer, implying a delay time in the
> milliseconds. Would that be acceptable in those cases?

A significant portion of the use cases should include mobile devices.
Device sales outnumbered desktop and server sales several years ago.

Many devices are sensor rich. Even the low-end ones come with
accelorometers for gaming. A typical one has 3 or 4 sensors, and
higher-end ones have 7 or 8 sensors. An Evo 4G has 7 of them.

There's no wanting for entropy in many of the use cases. The thing
that is lacking seems to be taking advantage of it.

Jeff