diff mbox series

crypto: qcom-rng: ensure buffer for generate is completely filled

Message ID 20220310232459.749638-1-bmasney@redhat.com
State Accepted
Commit a680b1832ced3b5fa7c93484248fd221ea0d614b
Headers show
Series crypto: qcom-rng: ensure buffer for generate is completely filled | expand

Commit Message

Brian Masney March 10, 2022, 11:24 p.m. UTC
The generate function in struct rng_alg expects that the destination
buffer is completely filled if the function returns 0. qcom_rng_read()
can run into a situation where the buffer is partially filled with
randomness and the remaining part of the buffer is zeroed since
qcom_rng_generate() doesn't check the return value. This issue can
be reproduced by running the following from libkcapi:

    kcapi-rng -b 9000000 > OUTFILE

The generated OUTFILE will have three huge sections that contain all
zeros, and this is caused by the code where the test
'val & PRNG_STATUS_DATA_AVAIL' fails.

Let's fix this issue by ensuring that qcom_rng_read() always returns
with a full buffer if the function returns success. Let's also have
qcom_rng_generate() return the correct value.

Here's some statistics from the ent project
(https://www.fourmilab.ch/random/) that shows information about the
quality of the generated numbers:

    $ ent -c qcom-random-before
    Value Char Occurrences Fraction
      0           606748   0.067416
      1            33104   0.003678
      2            33001   0.003667
    ...
    253   �        32883   0.003654
    254   �        33035   0.003671
    255   �        33239   0.003693

    Total:       9000000   1.000000

    Entropy = 7.811590 bits per byte.

    Optimum compression would reduce the size
    of this 9000000 byte file by 2 percent.

    Chi square distribution for 9000000 samples is 9329962.81, and
    randomly would exceed this value less than 0.01 percent of the
    times.

    Arithmetic mean value of data bytes is 119.3731 (127.5 = random).
    Monte Carlo value for Pi is 3.197293333 (error 1.77 percent).
    Serial correlation coefficient is 0.159130 (totally uncorrelated =
    0.0).

Without this patch, the results of the chi-square test is 0.01%, and
the numbers are certainly not random according to ent's project page.
The results improve with this patch:

    $ ent -c qcom-random-after
    Value Char Occurrences Fraction
      0            35432   0.003937
      1            35127   0.003903
      2            35424   0.003936
    ...
    253   �        35201   0.003911
    254   �        34835   0.003871
    255   �        35368   0.003930

    Total:       9000000   1.000000

    Entropy = 7.999979 bits per byte.

    Optimum compression would reduce the size
    of this 9000000 byte file by 0 percent.

    Chi square distribution for 9000000 samples is 258.77, and randomly
    would exceed this value 42.24 percent of the times.

    Arithmetic mean value of data bytes is 127.5006 (127.5 = random).
    Monte Carlo value for Pi is 3.141277333 (error 0.01 percent).
    Serial correlation coefficient is 0.000468 (totally uncorrelated =
    0.0).

This change was tested on a Nexus 5 phone (msm8974 SoC).

Signed-off-by: Brian Masney <bmasney@redhat.com>
Fixes: ceec5f5b5988 ("crypto: qcom-rng - Add Qcom prng driver")
Cc: stable@vger.kernel.org # 4.19+
---
 drivers/crypto/qcom-rng.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Bjorn Andersson March 11, 2022, 3:48 a.m. UTC | #1
On Thu 10 Mar 15:24 PST 2022, Brian Masney wrote:

> The generate function in struct rng_alg expects that the destination
> buffer is completely filled if the function returns 0. qcom_rng_read()
> can run into a situation where the buffer is partially filled with
> randomness and the remaining part of the buffer is zeroed since
> qcom_rng_generate() doesn't check the return value. This issue can
> be reproduced by running the following from libkcapi:
> 
>     kcapi-rng -b 9000000 > OUTFILE
> 
> The generated OUTFILE will have three huge sections that contain all
> zeros, and this is caused by the code where the test
> 'val & PRNG_STATUS_DATA_AVAIL' fails.
> 
> Let's fix this issue by ensuring that qcom_rng_read() always returns
> with a full buffer if the function returns success. Let's also have
> qcom_rng_generate() return the correct value.
> 
> Here's some statistics from the ent project
> (https://www.fourmilab.ch/random/) that shows information about the
> quality of the generated numbers:
> 
>     $ ent -c qcom-random-before
>     Value Char Occurrences Fraction
>       0           606748   0.067416
>       1            33104   0.003678
>       2            33001   0.003667
>     ...
>     253   ???        32883   0.003654
>     254   ???        33035   0.003671
>     255   ???        33239   0.003693
> 
>     Total:       9000000   1.000000
> 
>     Entropy = 7.811590 bits per byte.
> 
>     Optimum compression would reduce the size
>     of this 9000000 byte file by 2 percent.
> 
>     Chi square distribution for 9000000 samples is 9329962.81, and
>     randomly would exceed this value less than 0.01 percent of the
>     times.
> 
>     Arithmetic mean value of data bytes is 119.3731 (127.5 = random).
>     Monte Carlo value for Pi is 3.197293333 (error 1.77 percent).
>     Serial correlation coefficient is 0.159130 (totally uncorrelated =
>     0.0).
> 
> Without this patch, the results of the chi-square test is 0.01%, and
> the numbers are certainly not random according to ent's project page.
> The results improve with this patch:
> 
>     $ ent -c qcom-random-after
>     Value Char Occurrences Fraction
>       0            35432   0.003937
>       1            35127   0.003903
>       2            35424   0.003936
>     ...
>     253   ???        35201   0.003911
>     254   ???        34835   0.003871
>     255   ???        35368   0.003930
> 
>     Total:       9000000   1.000000
> 
>     Entropy = 7.999979 bits per byte.
> 
>     Optimum compression would reduce the size
>     of this 9000000 byte file by 0 percent.
> 
>     Chi square distribution for 9000000 samples is 258.77, and randomly
>     would exceed this value 42.24 percent of the times.
> 
>     Arithmetic mean value of data bytes is 127.5006 (127.5 = random).
>     Monte Carlo value for Pi is 3.141277333 (error 0.01 percent).
>     Serial correlation coefficient is 0.000468 (totally uncorrelated =
>     0.0).
> 
> This change was tested on a Nexus 5 phone (msm8974 SoC).
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> Fixes: ceec5f5b5988 ("crypto: qcom-rng - Add Qcom prng driver")
> Cc: stable@vger.kernel.org # 4.19+

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/crypto/qcom-rng.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
> index 99ba8d51d102..11f30fd48c14 100644
> --- a/drivers/crypto/qcom-rng.c
> +++ b/drivers/crypto/qcom-rng.c
> @@ -8,6 +8,7 @@
>  #include <linux/clk.h>
>  #include <linux/crypto.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -43,16 +44,19 @@ static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
>  {
>  	unsigned int currsize = 0;
>  	u32 val;
> +	int ret;
>  
>  	/* read random data from hardware */
>  	do {
> -		val = readl_relaxed(rng->base + PRNG_STATUS);
> -		if (!(val & PRNG_STATUS_DATA_AVAIL))
> -			break;
> +		ret = readl_poll_timeout(rng->base + PRNG_STATUS, val,
> +					 val & PRNG_STATUS_DATA_AVAIL,
> +					 200, 10000);
> +		if (ret)
> +			return ret;
>  
>  		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
>  		if (!val)
> -			break;
> +			return -EINVAL;
>  
>  		if ((max - currsize) >= WORD_SZ) {
>  			memcpy(data, &val, WORD_SZ);
> @@ -61,11 +65,10 @@ static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
>  		} else {
>  			/* copy only remaining bytes */
>  			memcpy(data, &val, max - currsize);
> -			break;
>  		}
>  	} while (currsize < max);
>  
> -	return currsize;
> +	return 0;
>  }
>  
>  static int qcom_rng_generate(struct crypto_rng *tfm,
> @@ -87,7 +90,7 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
>  	mutex_unlock(&rng->lock);
>  	clk_disable_unprepare(rng->clk);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int qcom_rng_seed(struct crypto_rng *tfm, const u8 *seed,
> -- 
> 2.34.1
>
Andrew Halaney March 11, 2022, 1:31 p.m. UTC | #2
On Thu, Mar 10, 2022 at 06:24:59PM -0500, Brian Masney wrote:
> The generate function in struct rng_alg expects that the destination
> buffer is completely filled if the function returns 0. qcom_rng_read()
> can run into a situation where the buffer is partially filled with
> randomness and the remaining part of the buffer is zeroed since
> qcom_rng_generate() doesn't check the return value. This issue can
> be reproduced by running the following from libkcapi:
> 
>     kcapi-rng -b 9000000 > OUTFILE
> 
> The generated OUTFILE will have three huge sections that contain all
> zeros, and this is caused by the code where the test
> 'val & PRNG_STATUS_DATA_AVAIL' fails.
> 
> Let's fix this issue by ensuring that qcom_rng_read() always returns
> with a full buffer if the function returns success. Let's also have
> qcom_rng_generate() return the correct value.
> 
> Here's some statistics from the ent project
> (https://www.fourmilab.ch/random/) that shows information about the
> quality of the generated numbers:
> 
>     $ ent -c qcom-random-before
>     Value Char Occurrences Fraction
>       0           606748   0.067416
>       1            33104   0.003678
>       2            33001   0.003667
>     ...
>     253   �        32883   0.003654
>     254   �        33035   0.003671
>     255   �        33239   0.003693
> 
>     Total:       9000000   1.000000
> 
>     Entropy = 7.811590 bits per byte.
> 
>     Optimum compression would reduce the size
>     of this 9000000 byte file by 2 percent.
> 
>     Chi square distribution for 9000000 samples is 9329962.81, and
>     randomly would exceed this value less than 0.01 percent of the
>     times.
> 
>     Arithmetic mean value of data bytes is 119.3731 (127.5 = random).
>     Monte Carlo value for Pi is 3.197293333 (error 1.77 percent).
>     Serial correlation coefficient is 0.159130 (totally uncorrelated =
>     0.0).
> 
> Without this patch, the results of the chi-square test is 0.01%, and
> the numbers are certainly not random according to ent's project page.
> The results improve with this patch:
> 
>     $ ent -c qcom-random-after
>     Value Char Occurrences Fraction
>       0            35432   0.003937
>       1            35127   0.003903
>       2            35424   0.003936
>     ...
>     253   �        35201   0.003911
>     254   �        34835   0.003871
>     255   �        35368   0.003930
> 
>     Total:       9000000   1.000000
> 
>     Entropy = 7.999979 bits per byte.
> 
>     Optimum compression would reduce the size
>     of this 9000000 byte file by 0 percent.
> 
>     Chi square distribution for 9000000 samples is 258.77, and randomly
>     would exceed this value 42.24 percent of the times.
> 
>     Arithmetic mean value of data bytes is 127.5006 (127.5 = random).
>     Monte Carlo value for Pi is 3.141277333 (error 0.01 percent).
>     Serial correlation coefficient is 0.000468 (totally uncorrelated =
>     0.0).
> 
> This change was tested on a Nexus 5 phone (msm8974 SoC).
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> Fixes: ceec5f5b5988 ("crypto: qcom-rng - Add Qcom prng driver")
> Cc: stable@vger.kernel.org # 4.19+

FWIW

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/crypto/qcom-rng.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
> index 99ba8d51d102..11f30fd48c14 100644
> --- a/drivers/crypto/qcom-rng.c
> +++ b/drivers/crypto/qcom-rng.c
> @@ -8,6 +8,7 @@
>  #include <linux/clk.h>
>  #include <linux/crypto.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -43,16 +44,19 @@ static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
>  {
>  	unsigned int currsize = 0;
>  	u32 val;
> +	int ret;
>  
>  	/* read random data from hardware */
>  	do {
> -		val = readl_relaxed(rng->base + PRNG_STATUS);
> -		if (!(val & PRNG_STATUS_DATA_AVAIL))
> -			break;
> +		ret = readl_poll_timeout(rng->base + PRNG_STATUS, val,
> +					 val & PRNG_STATUS_DATA_AVAIL,
> +					 200, 10000);
> +		if (ret)
> +			return ret;
>  
>  		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
>  		if (!val)
> -			break;
> +			return -EINVAL;
>  
>  		if ((max - currsize) >= WORD_SZ) {
>  			memcpy(data, &val, WORD_SZ);
> @@ -61,11 +65,10 @@ static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
>  		} else {
>  			/* copy only remaining bytes */
>  			memcpy(data, &val, max - currsize);
> -			break;
>  		}
>  	} while (currsize < max);
>  
> -	return currsize;
> +	return 0;
>  }
>  
>  static int qcom_rng_generate(struct crypto_rng *tfm,
> @@ -87,7 +90,7 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
>  	mutex_unlock(&rng->lock);
>  	clk_disable_unprepare(rng->clk);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int qcom_rng_seed(struct crypto_rng *tfm, const u8 *seed,
> -- 
> 2.34.1
>
Herbert Xu March 14, 2022, 2:54 a.m. UTC | #3
On Thu, Mar 10, 2022 at 06:24:59PM -0500, Brian Masney wrote:
> The generate function in struct rng_alg expects that the destination
> buffer is completely filled if the function returns 0. qcom_rng_read()
> can run into a situation where the buffer is partially filled with
> randomness and the remaining part of the buffer is zeroed since
> qcom_rng_generate() doesn't check the return value. This issue can
> be reproduced by running the following from libkcapi:
> 
>     kcapi-rng -b 9000000 > OUTFILE
> 
> The generated OUTFILE will have three huge sections that contain all
> zeros, and this is caused by the code where the test
> 'val & PRNG_STATUS_DATA_AVAIL' fails.
> 
> Let's fix this issue by ensuring that qcom_rng_read() always returns
> with a full buffer if the function returns success. Let's also have
> qcom_rng_generate() return the correct value.
> 
> Here's some statistics from the ent project
> (https://www.fourmilab.ch/random/) that shows information about the
> quality of the generated numbers:
> 
>     $ ent -c qcom-random-before
>     Value Char Occurrences Fraction
>       0           606748   0.067416
>       1            33104   0.003678
>       2            33001   0.003667
>     ...
>     253   �        32883   0.003654
>     254   �        33035   0.003671
>     255   �        33239   0.003693
> 
>     Total:       9000000   1.000000
> 
>     Entropy = 7.811590 bits per byte.
> 
>     Optimum compression would reduce the size
>     of this 9000000 byte file by 2 percent.
> 
>     Chi square distribution for 9000000 samples is 9329962.81, and
>     randomly would exceed this value less than 0.01 percent of the
>     times.
> 
>     Arithmetic mean value of data bytes is 119.3731 (127.5 = random).
>     Monte Carlo value for Pi is 3.197293333 (error 1.77 percent).
>     Serial correlation coefficient is 0.159130 (totally uncorrelated =
>     0.0).
> 
> Without this patch, the results of the chi-square test is 0.01%, and
> the numbers are certainly not random according to ent's project page.
> The results improve with this patch:
> 
>     $ ent -c qcom-random-after
>     Value Char Occurrences Fraction
>       0            35432   0.003937
>       1            35127   0.003903
>       2            35424   0.003936
>     ...
>     253   �        35201   0.003911
>     254   �        34835   0.003871
>     255   �        35368   0.003930
> 
>     Total:       9000000   1.000000
> 
>     Entropy = 7.999979 bits per byte.
> 
>     Optimum compression would reduce the size
>     of this 9000000 byte file by 0 percent.
> 
>     Chi square distribution for 9000000 samples is 258.77, and randomly
>     would exceed this value 42.24 percent of the times.
> 
>     Arithmetic mean value of data bytes is 127.5006 (127.5 = random).
>     Monte Carlo value for Pi is 3.141277333 (error 0.01 percent).
>     Serial correlation coefficient is 0.000468 (totally uncorrelated =
>     0.0).
> 
> This change was tested on a Nexus 5 phone (msm8974 SoC).
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> Fixes: ceec5f5b5988 ("crypto: qcom-rng - Add Qcom prng driver")
> Cc: stable@vger.kernel.org # 4.19+
> ---
>  drivers/crypto/qcom-rng.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

Patch applied.  Thanks.
diff mbox series

Patch

diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
index 99ba8d51d102..11f30fd48c14 100644
--- a/drivers/crypto/qcom-rng.c
+++ b/drivers/crypto/qcom-rng.c
@@ -8,6 +8,7 @@ 
 #include <linux/clk.h>
 #include <linux/crypto.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -43,16 +44,19 @@  static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
 {
 	unsigned int currsize = 0;
 	u32 val;
+	int ret;
 
 	/* read random data from hardware */
 	do {
-		val = readl_relaxed(rng->base + PRNG_STATUS);
-		if (!(val & PRNG_STATUS_DATA_AVAIL))
-			break;
+		ret = readl_poll_timeout(rng->base + PRNG_STATUS, val,
+					 val & PRNG_STATUS_DATA_AVAIL,
+					 200, 10000);
+		if (ret)
+			return ret;
 
 		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
 		if (!val)
-			break;
+			return -EINVAL;
 
 		if ((max - currsize) >= WORD_SZ) {
 			memcpy(data, &val, WORD_SZ);
@@ -61,11 +65,10 @@  static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
 		} else {
 			/* copy only remaining bytes */
 			memcpy(data, &val, max - currsize);
-			break;
 		}
 	} while (currsize < max);
 
-	return currsize;
+	return 0;
 }
 
 static int qcom_rng_generate(struct crypto_rng *tfm,
@@ -87,7 +90,7 @@  static int qcom_rng_generate(struct crypto_rng *tfm,
 	mutex_unlock(&rng->lock);
 	clk_disable_unprepare(rng->clk);
 
-	return 0;
+	return ret;
 }
 
 static int qcom_rng_seed(struct crypto_rng *tfm, const u8 *seed,