A few issues with jitterentropy_rng

Commit Message

Alexander Patrakov Sept. 17, 2019, 4:07 p.m. UTC

Hello.

While reviewing the code for jitterentropy-rng, I found a few issues.

1. (linux-mips: that's why you are in the CC: of this mail) The code in 
crypto/jitterentropy.c uses the jent_get_nstime() function as a source 
of a high-resolution timestamp, and contains a check that the time does 
not go backwards too often. The implementation (defined in 
crypto/jitterentropy-kcapi.c) uses random_get_entropy() and, if it 
returns 0, falls back to ktime_get_ns().

The problem is - while the generic implementation of 
random_get_entropy() is indeed another name for get_cycles() and _is_ a 
monotonically increasing counter if implemented, this may not be true 
for random_get_entropy() on mips, because it has a fallback to 
read_c0_random(), which is not a cycle counter. This fallback to 
read_c0_random(), if I understand correctly, by virtue of returning 
non-zero, would prevent a more useful (?) fallback to ktime_get_ns() in 
jent_get_nstime().

2. There are some outdated/incorrect comments in crypto/jitterentropy.c 
(patch attached).

2a. Above jent_lfsr_time(): the comment says that ec may be NULL, but 
the function dereferences it unconditionally.

2b. Above jent_fips_test(): the function returns void, but the comment 
talks about a zero vs negative return value. It would be correct here to 
talk about returning normally vs causing a kernel panic.

Comments

Stephan Mueller Sept. 18, 2019, 4:03 a.m. UTC | #1

Am Dienstag, 17. September 2019, 18:07:36 CEST schrieb Alexander E. Patrakov:

Hi Alexander,

> Hello.
> 
> While reviewing the code for jitterentropy-rng, I found a few issues.
> 
> 1. (linux-mips: that's why you are in the CC: of this mail) The code in
> crypto/jitterentropy.c uses the jent_get_nstime() function as a source
> of a high-resolution timestamp, and contains a check that the time does
> not go backwards too often. The implementation (defined in
> crypto/jitterentropy-kcapi.c) uses random_get_entropy() and, if it
> returns 0, falls back to ktime_get_ns().
> 
> The problem is - while the generic implementation of
> random_get_entropy() is indeed another name for get_cycles() and _is_ a
> monotonically increasing counter if implemented, this may not be true
> for random_get_entropy() on mips, because it has a fallback to
> read_c0_random(), which is not a cycle counter. This fallback to
> read_c0_random(), if I understand correctly, by virtue of returning
> non-zero, would prevent a more useful (?) fallback to ktime_get_ns() in
> jent_get_nstime().

Ok. I fail to understand what is the issue. Do you see any errors that you 
think you should not see?
> 
> 2. There are some outdated/incorrect comments in crypto/jitterentropy.c
> (patch attached).
> 
> 2a. Above jent_lfsr_time(): the comment says that ec may be NULL, but
> the function dereferences it unconditionally.

You are right, this is a copy and paste error from the user space code.
> 
> 2b. Above jent_fips_test(): the function returns void, but the comment
> talks about a zero vs negative return value. It would be correct here to
> talk about returning normally vs causing a kernel panic.

Yes, again a copy and paste error from user space. 

Could you please send the patch as a patch request to linux-crypto?

Ciao
Stephan

Alexander Patrakov Sept. 18, 2019, 6:45 a.m. UTC | #2

18.09.2019 09:03, Stephan Müller пишет:
> Am Dienstag, 17. September 2019, 18:07:36 CEST schrieb Alexander E. Patrakov:
> 
> Hi Alexander,
> 
>> Hello.
>>
>> While reviewing the code for jitterentropy-rng, I found a few issues.
>>
>> 1. (linux-mips: that's why you are in the CC: of this mail) The code in
>> crypto/jitterentropy.c uses the jent_get_nstime() function as a source
>> of a high-resolution timestamp, and contains a check that the time does
>> not go backwards too often. The implementation (defined in
>> crypto/jitterentropy-kcapi.c) uses random_get_entropy() and, if it
>> returns 0, falls back to ktime_get_ns().
>>
>> The problem is - while the generic implementation of
>> random_get_entropy() is indeed another name for get_cycles() and _is_ a
>> monotonically increasing counter if implemented, this may not be true
>> for random_get_entropy() on mips, because it has a fallback to
>> read_c0_random(), which is not a cycle counter. This fallback to
>> read_c0_random(), if I understand correctly, by virtue of returning
>> non-zero, would prevent a more useful (?) fallback to ktime_get_ns() in
>> jent_get_nstime().
> 
> Ok. I fail to understand what is the issue. Do you see any errors that you
> think you should not see?

The issue was found by code inspection. I do not have hardware that 
reproduces it. But the error would be:

jitterentropy: Initialization failed with host not compliant with 
requirements: 3

...while a fallback to ktime_get_ns() instead of random_get_entropy() 
would have produced a working jitterentropy. Again, the issue is 
mips-specific, and would appear only on hardware where 
can_use_mips_counter() returns 0, read_c0_prid() is neither 
PRID_IMP_R6000 nor PRID_IMP_R6000A, and ktime_get_ns() is not too coarse.

>>
>> 2. There are some outdated/incorrect comments in crypto/jitterentropy.c
>> (patch attached).
>>
>> 2a. Above jent_lfsr_time(): the comment says that ec may be NULL, but
>> the function dereferences it unconditionally.
> 
> You are right, this is a copy and paste error from the user space code.
>>
>> 2b. Above jent_fips_test(): the function returns void, but the comment
>> talks about a zero vs negative return value. It would be correct here to
>> talk about returning normally vs causing a kernel panic.
> 
> Yes, again a copy and paste error from user space.
> 
> Could you please send the patch as a patch request to linux-crypto?

Will do so.

Patch

From cbbdb4bc2c428f8ccd684ebd99b287689c4a625c Mon Sep 17 00:00:00 2001
From: "Alexander E. Patrakov" <[e-mail]>
Date: Mon, 16 Sep 2019 09:36:30 +0500
Subject: [PATCH] jitterentropy: fix comments

One should not say "ec can be NULL" and then dereference it.
One cannot talk about the return value if the function returns void.

Signed-off-by: Alexander E. Patrakov <[e-mail]>
---
 crypto/jitterentropy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
index 77fa2120fe0c..9597f9f5723d 100644
--- a/crypto/jitterentropy.c
+++ b/crypto/jitterentropy.c
@@ -172,7 +172,7 @@  static __u64 jent_loop_shuffle(struct rand_data *ec,
  * implies that careful retesting must be done.
  *
  * Input:
- * @ec entropy collector struct -- may be NULL
+ * @ec entropy collector struct
  * @time time stamp to be injected
  * @loop_cnt if a value not equal to 0 is set, use the given value as number of
  *	     loops to perform the folding
@@ -400,8 +400,8 @@  static void jent_gen_entropy(struct rand_data *ec)
  * primes the test if needed.
  *
  * Return:
- * 0 if FIPS test passed
- * < 0 if FIPS test failed
+ * returns normally if FIPS test passed
+ * panics the kernel if FIPS test failed
  */
 static void jent_fips_test(struct rand_data *ec)
 {
-- 
2.23.0