| Message ID | 20220310232459.749638-1-bmasney@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: qcom-rng: ensure buffer for generate is completely filled | expand |
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 >
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 >
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 --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,
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(-)