| Message ID | 20180618141259.23141-4-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote: > Qcom 8996 and later chips support prng v2 where we need to only > implement .read callback for hwrng. > The hardware still needs initialization, so I think you should expand this to mention that the initialization is moved to secure world and that's the reason why we only implement read. The question is what happens in projects with other security models. > Add a new table for v2 which supports this and get version required for > driver data. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/char/hw_random/msm-rng.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c > index 7644474035e5..3f509072a6c6 100644 > --- a/drivers/char/hw_random/msm-rng.c > +++ b/drivers/char/hw_random/msm-rng.c > @@ -17,6 +17,7 @@ > #include <linux/io.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > > /* Device specific register offsets */ > @@ -132,10 +133,16 @@ static struct hwrng msm_rng = { > .read = msm_rng_read, > }; > > +static struct hwrng msm_rng_v2 = { > + .name = KBUILD_MODNAME, > + .read = msm_rng_read, > +}; > + > static int msm_rng_probe(struct platform_device *pdev) > { > struct resource *res; > struct msm_rng *rng; > + unsigned int version; > int ret; > > rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL); > @@ -154,6 +161,9 @@ static int msm_rng_probe(struct platform_device *pdev) > return PTR_ERR(rng->clk); > > rng->hwrng = &msm_rng; > + version = (unsigned long)of_device_get_match_data(&pdev->dev); If this is "version" then please make it 1 or 2, if you agree with Stephen's suggestion of omitting the initialization of init I think this would be better as 0/1 and the variable named "skip_init". > + if (version) > + rng->hwrng = &msm_rng_v2; > > rng->hwrng->priv = (unsigned long)rng; > ret = devm_hwrng_register(&pdev->dev, rng->hwrng); > @@ -166,7 +176,8 @@ static int msm_rng_probe(struct platform_device *pdev) > } > > static const struct of_device_id msm_rng_of_match[] = { > - { .compatible = "qcom,prng", }, > + { .compatible = "qcom,prng", .data = (void *)0}, > + { .compatible = "qcom,prng-v2", .data = (void *)1}, > {} > }; > MODULE_DEVICE_TABLE(of, msm_rng_of_match); Regards, Bjorn
Quoting Bjorn Andersson (2018-06-18 11:21:23) > On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote: > > > Qcom 8996 and later chips support prng v2 where we need to only > > implement .read callback for hwrng. > > > > The hardware still needs initialization, so I think you should expand > this to mention that the initialization is moved to secure world and > that's the reason why we only implement read. > > The question is what happens in projects with other security models. Can we still read the PRNG_CONFIG register to see if it's already been configured or not and then bail out if it isn't configured? That would be better as long as we the system doesn't blow up if non-secure mode tries to read the config register.
On 18-06-18, 11:21, Bjorn Andersson wrote: > On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote: > > > Qcom 8996 and later chips support prng v2 where we need to only > > implement .read callback for hwrng. > > > > The hardware still needs initialization, so I think you should expand > this to mention that the initialization is moved to secure world and > that's the reason why we only implement read. > > The question is what happens in projects with other security models. I did think about that, in those case a DT change would do the trick by pointing to the TZ EE and loading v1 driver but then is DT board sepcific or SoC specific, I think latter :( Can we detect the model..? > > rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL); > > @@ -154,6 +161,9 @@ static int msm_rng_probe(struct platform_device *pdev) > > return PTR_ERR(rng->clk); > > > > rng->hwrng = &msm_rng; > > + version = (unsigned long)of_device_get_match_data(&pdev->dev); > > If this is "version" then please make it 1 or 2, if you agree with > Stephen's suggestion of omitting the initialization of init I think this > would be better as 0/1 and the variable named "skip_init". ok will do
On 18-06-18, 13:24, Stephen Boyd wrote: > Quoting Bjorn Andersson (2018-06-18 11:21:23) > > On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote: > > > > > Qcom 8996 and later chips support prng v2 where we need to only > > > implement .read callback for hwrng. > > > > > > > The hardware still needs initialization, so I think you should expand > > this to mention that the initialization is moved to secure world and > > that's the reason why we only implement read. > > > > The question is what happens in projects with other security models. > > Can we still read the PRNG_CONFIG register to see if it's already been > configured or not and then bail out if it isn't configured? That would > be better as long as we the system doesn't blow up if non-secure mode > tries to read the config register. Unfortunately it did blow up for me when I tried it..
On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote: > Qcom 8996 and later chips support prng v2 where we need to only > implement .read callback for hwrng. > > Add a new table for v2 which supports this and get version required for > driver data. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> Is this really a pseudo-RNG? If so it needs to be moved over to the algif_rng interface. hwrng is for true hardware RNGs only, because it may be directly fed into /dev/random. Thanks,
On 19-06-18, 22:28, Herbert Xu wrote: > On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote: > > Qcom 8996 and later chips support prng v2 where we need to only > > implement .read callback for hwrng. > > > > Add a new table for v2 which supports this and get version required for > > driver data. > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > Is this really a pseudo-RNG? If so it needs to be moved over to > the algif_rng interface. > > hwrng is for true hardware RNGs only, because it may be directly > fed into /dev/random. I am trying to find how how much random output this hardware generates. Meanwhile, can you please point out examples/Documentation for algif_rng and if any test tools for this? Thanks
On Wed, Jun 20, 2018 at 11:02:04AM +0530, Vinod wrote: > > I am trying to find how how much random output this hardware generates. > > Meanwhile, can you please point out examples/Documentation for algif_rng > and if any test tools for this? Have a look at drivers/crypto/exynos-rng.c. Cheers,
Hi Vinod, On 20 June 2018 at 11:02, Vinod <vkoul@kernel.org> wrote: > On 19-06-18, 22:28, Herbert Xu wrote: >> On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote: >> > Qcom 8996 and later chips support prng v2 where we need to only >> > implement .read callback for hwrng. >> > >> > Add a new table for v2 which supports this and get version required for >> > driver data. >> > >> > Signed-off-by: Vinod Koul <vkoul@kernel.org> >> >> Is this really a pseudo-RNG? If so it needs to be moved over to >> the algif_rng interface. >> >> hwrng is for true hardware RNGs only, because it may be directly >> fed into /dev/random. > > I am trying to find how how much random output this hardware generates. > > Meanwhile, can you please point out examples/Documentation for algif_rng > and if any test tools for this? As Herbert suggested exynos rng is a good example. It was wrongly using hwrng framework then switched to using crypto framework. I have a patch for msm rng using crypto framework, only compile tested though. You can find the patch at https://pastebin.ubuntu.com/p/44r6BJgBkN/. I am having a hard time sending patches via git send-email form a gmail account so I have put it in pastebin. Feel free to use it if that is useful. Regards, PrasannaKumar
Hi PrasannaKumar, On 20-06-18, 23:15, PrasannaKumar Muralidharan wrote: > Hi Vinod, > > On 20 June 2018 at 11:02, Vinod <vkoul@kernel.org> wrote: > > On 19-06-18, 22:28, Herbert Xu wrote: > >> On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote: > >> > Qcom 8996 and later chips support prng v2 where we need to only > >> > implement .read callback for hwrng. > >> > > >> > Add a new table for v2 which supports this and get version required for > >> > driver data. > >> > > >> > Signed-off-by: Vinod Koul <vkoul@kernel.org> > >> > >> Is this really a pseudo-RNG? If so it needs to be moved over to > >> the algif_rng interface. > >> > >> hwrng is for true hardware RNGs only, because it may be directly > >> fed into /dev/random. > > > > I am trying to find how how much random output this hardware generates. > > > > Meanwhile, can you please point out examples/Documentation for algif_rng > > and if any test tools for this? > > As Herbert suggested exynos rng is a good example. It was wrongly > using hwrng framework then switched to using crypto framework. > > I have a patch for msm rng using crypto framework, only compile tested > though. You can find the patch at > https://pastebin.ubuntu.com/p/44r6BJgBkN/. I am having a hard time > sending patches via git send-email form a gmail account so I have put > it in pastebin. Feel free to use it if that is useful. Ah, I already started based on msm downstream implementation (they had one using crypto APIs too). Ideally you should have posted it and tested it. Thanks
Hi Herbert, On 06/19/2018 05:28 PM, Herbert Xu wrote: > On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote: >> Qcom 8996 and later chips support prng v2 where we need to only >> implement .read callback for hwrng. >> >> Add a new table for v2 which supports this and get version required for >> driver data. >> >> Signed-off-by: Vinod Koul <vkoul@kernel.org> > > Is this really a pseudo-RNG? If so it needs to be moved over to > the algif_rng interface. Despite the register name (PRNG_ registers prefix) the IP is using FIPS approved algorithm and we can claim that this is true hardware entropy generator. I don't think that we need to move to algif_rng.
On Thu, Jun 21, 2018 at 12:56:34PM +0300, Stanimir Varbanov wrote: > > > Is this really a pseudo-RNG? If so it needs to be moved over to > > the algif_rng interface. > > Despite the register name (PRNG_ registers prefix) the IP is using FIPS > approved algorithm and we can claim that this is true hardware entropy > generator. Whether your RNG is a PRNG or not has nothing to do with the algorithm you use. No algorithm can generate entropy out of thin air. Cheers,
Hi Herbert, On 06/21/2018 01:15 PM, Herbert Xu wrote: > On Thu, Jun 21, 2018 at 12:56:34PM +0300, Stanimir Varbanov wrote: >> >>> Is this really a pseudo-RNG? If so it needs to be moved over to >>> the algif_rng interface. >> >> Despite the register name (PRNG_ registers prefix) the IP is using FIPS >> approved algorithm and we can claim that this is true hardware entropy >> generator. > > Whether your RNG is a PRNG or not has nothing to do with the > algorithm you use. No algorithm can generate entropy out of thin > air. OK, I just wanted to say that it is _not_ PRNG and the register names gives us wrong impression.
On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: > > OK, I just wanted to say that it is _not_ PRNG and the register names > gives us wrong impression. So does it generate one bit of output for each bit of hardware- generated entropy like /dev/random? Or does it use a hardware- generated seed to power a PRNG? Cheers,
Hi Herbert, On 06/21/2018 02:53 PM, Herbert Xu wrote: > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: >> >> OK, I just wanted to say that it is _not_ PRNG and the register names >> gives us wrong impression. > > So does it generate one bit of output for each bit of hardware- > generated entropy like /dev/random? Or does it use a hardware- > generated seed to power a PRNG? I believe it is the second one. Isn't the second one SP 800-90A?
On Fri, Jun 22, 2018 at 11:27:59AM +0300, Stanimir Varbanov wrote: > Hi Herbert, > > On 06/21/2018 02:53 PM, Herbert Xu wrote: > > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: > >> > >> OK, I just wanted to say that it is _not_ PRNG and the register names > >> gives us wrong impression. > > > > So does it generate one bit of output for each bit of hardware- > > generated entropy like /dev/random? Or does it use a hardware- > > generated seed to power a PRNG? > > I believe it is the second one. Isn't the second one SP 800-90A? In that case it should switch over to algif_rng. Thanks,
On 22-06-18, 22:38, Herbert Xu wrote: > On Fri, Jun 22, 2018 at 11:27:59AM +0300, Stanimir Varbanov wrote: > > Hi Herbert, > > > > On 06/21/2018 02:53 PM, Herbert Xu wrote: > > > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: > > >> > > >> OK, I just wanted to say that it is _not_ PRNG and the register names > > >> gives us wrong impression. > > > > > > So does it generate one bit of output for each bit of hardware- > > > generated entropy like /dev/random? Or does it use a hardware- > > > generated seed to power a PRNG? > > > > I believe it is the second one. Isn't the second one SP 800-90A? > > In that case it should switch over to algif_rng. Okay I am doing the port taking the exynos-rng as a ref. Question is how to test it, how is one supposed to exercise the rng, any test utils/apps for that? Sorry for noob question, new to crypto interfaces.
On Fri, Jun 22, 2018 at 08:18:09PM +0530, Vinod wrote: > > Okay I am doing the port taking the exynos-rng as a ref. > Question is how to test it, how is one supposed to exercise the rng, any > test utils/apps for that? Sorry for noob question, new to crypto > interfaces. algif_rng is available through the af_alg socket interface. Ccing Stephan as he has a library that may help you. Cheers,
Hi, On 06/22/2018 05:38 PM, Herbert Xu wrote: > On Fri, Jun 22, 2018 at 11:27:59AM +0300, Stanimir Varbanov wrote: >> Hi Herbert, >> >> On 06/21/2018 02:53 PM, Herbert Xu wrote: >>> On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: >>>> >>>> OK, I just wanted to say that it is _not_ PRNG and the register names >>>> gives us wrong impression. >>> >>> So does it generate one bit of output for each bit of hardware- >>> generated entropy like /dev/random? Or does it use a hardware- >>> generated seed to power a PRNG? >> >> I believe it is the second one. Isn't the second one SP 800-90A? > > In that case it should switch over to algif_rng. OK, what about the rest drivers in drivers/char/hw_random? Are they all true RNG?
On 22-06-18, 19:57, Stephan Mueller wrote: > Hi > > > > Am 22.06.2018 um 16:50 schrieb Herbert Xu <herbert@gondor.apana.org.au>: > > > >> On Fri, Jun 22, 2018 at 08:18:09PM +0530, Vinod wrote: > >> > >> Okay I am doing the port taking the exynos-rng as a ref. > >> Question is how to test it, how is one supposed to exercise the rng, any > >> test utils/apps for that? Sorry for noob question, new to crypto > >> interfaces. > > > > algif_rng is available through the af_alg socket interface. > > > You can use the libkcapi library at http://www.chronox.de/libkcapi.html > > The RNG API is documented at http://chronox.de/libkcapi/html/ch03s15.html and http://chronox.de/libkcapi/html/ch03s16.html > > A command line app is also present with kcapi-rng as documented at https://github.com/smuellerDD/libkcapi/blob/master/README.md Thanks for the pointers, it helped me to test the driver :) I have two follow up question on crypto: - If there a way to avoid using a global variable in driver to hold the pointer for driver memory? Looks like exynos driver does that. I understand that the crypto callback don't provide driver context as they copy the data structures passed in registration API, but a simpler way to get driver context would be desirable. - .seed seems to be mandatory, if I do not set it and even use .seedsize = 0, it panics at crypto_rng_reset(). So is .seed mandatory? Thanks
Am Mittwoch, 27. Juni 2018, 07:08:53 CEST schrieb Vinod: Hi Vinod, > Thanks for the pointers, it helped me to test the driver :) > > I have two follow up question on crypto: > > - If there a way to avoid using a global variable in driver to hold the > pointer for driver memory? Looks like exynos driver does that. > > I understand that the crypto callback don't provide driver context as > they copy the data structures passed in registration API, but a simpler > way to get driver context would be desirable. Sure the kernel crypto API can and has to maintain a per-instance data structure. See the crypto/drbg.c for instance. static int drbg_kcapi_random(struct crypto_rng *tfm, const u8 *src, unsigned int slen, u8 *dst, unsigned int dlen) { struct drbg_state *drbg = crypto_rng_ctx(tfm); static int drbg_kcapi_seed(struct crypto_rng *tfm, const u8 *seed, unsigned int slen) { struct drbg_state *drbg = crypto_rng_ctx(tfm); The key is: alg->base.cra_ctxsize = sizeof(struct drbg_state); during initialization since the kernel crypto API allocates that buffer for you and releases it during deallocation. > > - .seed seems to be mandatory, if I do not set it and even use > .seedsize = 0, it panics at crypto_rng_reset(). So is .seed > mandatory? Well, seedsize = 0 just says that the RNG is ready to use after initialization (i.e. it does not need to be seeded after initialization). That does not preclude that a caller wants to reseed. And yes, .seed must be set. > > Thanks Ciao Stephan
Hi Stephan, Thanks for the answers, they are helpful. On 27-06-18, 08:13, Stephan Mueller wrote: > > I have two follow up question on crypto: > > > > - If there a way to avoid using a global variable in driver to hold the > > pointer for driver memory? Looks like exynos driver does that. > > > > I understand that the crypto callback don't provide driver context as > > they copy the data structures passed in registration API, but a simpler > > way to get driver context would be desirable. > > Sure the kernel crypto API can and has to maintain a per-instance data > structure. > > See the crypto/drbg.c for instance. > > static int drbg_kcapi_random(struct crypto_rng *tfm, > const u8 *src, unsigned int slen, > u8 *dst, unsigned int dlen) > { > struct drbg_state *drbg = crypto_rng_ctx(tfm); > > static int drbg_kcapi_seed(struct crypto_rng *tfm, > const u8 *seed, unsigned int slen) > { > struct drbg_state *drbg = crypto_rng_ctx(tfm); > > The key is: > > alg->base.cra_ctxsize = sizeof(struct drbg_state); > > during initialization since the kernel crypto API allocates that buffer for > you and releases it during deallocation. The difference here is that memory is allocated by crypto and driver has no way to pass "it's" own data while doing registration. Ideally registration should accept a pointer/long and pass that back on a callbacks Currently am doing bunch of initialization in .probe (platform driver) and I think recommendation would be to move that to .cra_init, which seem plausible but I don't have pdev to read hw_resource etc.. so would still need to get that. FWIW here is the code I wrote: https://git.linaro.org/people/vinod.koul/kernel.git/commit/?h=topic/rng_v2&id=feb23a41afb0d4cf42a2825b84a43dbc9a49e8b9
Am Mittwoch, 27. Juni 2018, 08:27:01 CEST schrieb Vinod: Hi Vinod, > Hi Stephan, > > Thanks for the answers, they are helpful. > > On 27-06-18, 08:13, Stephan Mueller wrote: > > > I have two follow up question on crypto: > > > - If there a way to avoid using a global variable in driver to hold the > > > > > > pointer for driver memory? Looks like exynos driver does that. > > > > > > I understand that the crypto callback don't provide driver context as > > > they copy the data structures passed in registration API, but a > > > simpler > > > way to get driver context would be desirable. > > > > Sure the kernel crypto API can and has to maintain a per-instance data > > structure. > > > > See the crypto/drbg.c for instance. > > > > static int drbg_kcapi_random(struct crypto_rng *tfm, > > > > const u8 *src, unsigned int slen, > > u8 *dst, unsigned int dlen) > > > > { > > > > struct drbg_state *drbg = crypto_rng_ctx(tfm); > > > > static int drbg_kcapi_seed(struct crypto_rng *tfm, > > > > const u8 *seed, unsigned int slen) > > > > { > > > > struct drbg_state *drbg = crypto_rng_ctx(tfm); > > > > The key is: > > alg->base.cra_ctxsize = sizeof(struct drbg_state); > > > > during initialization since the kernel crypto API allocates that buffer > > for > > you and releases it during deallocation. > > The difference here is that memory is allocated by crypto and driver has > no way to pass "it's" own data while doing registration. Ideally > registration should accept a pointer/long and pass that back on a > callbacks Looking at your code, it seems you do what makes sense: there is only one instance of the driver, if at all. Thus, having qcom_rng_dev as static makes sense. The kernel crypto API allows arbitrary instances of the RNG as well as frequent allocations and deallocations. And this is why there must be a disconnect between the one hardware-resource driver-instance data structure and the (potentially) multiple crypto API RNG instances and their data structures. > > Currently am doing bunch of initialization in .probe (platform driver) > and I think recommendation would be to move that to .cra_init, which seem > plausible but I don't have pdev to read hw_resource etc.. so would still > need to get that. It seems that your allocation during probe relates to the hardware resource where you only have one in the system. Thus, doing the allocation here makes sense. And, you do not want to perform probe or such resource allocation once per crypto API RNG instance allocation. As said, there can be multiple or even they can be allocated and deallocated frequently. This in particular applies if your driver's "stdrng" has the highest prio which means that it will be allocated and deallocated frequently. Ciao Stephan
Hi Stephan, Thanks for quick reply, On 27-06-18, 08:43, Stephan Mueller wrote: > > On 27-06-18, 08:13, Stephan Mueller wrote: > > > The key is: > > > alg->base.cra_ctxsize = sizeof(struct drbg_state); > > > > > > during initialization since the kernel crypto API allocates that buffer > > > for > > > you and releases it during deallocation. > > > > The difference here is that memory is allocated by crypto and driver has > > no way to pass "it's" own data while doing registration. Ideally > > registration should accept a pointer/long and pass that back on a > > callbacks > > Looking at your code, it seems you do what makes sense: there is only one > instance of the driver, if at all. Thus, having qcom_rng_dev as static makes > sense. The kernel crypto API allows arbitrary instances of the RNG as well as > frequent allocations and deallocations. And this is why there must be a > disconnect between the one hardware-resource driver-instance data structure > and the (potentially) multiple crypto API RNG instances and their data > structures. For now it is true, but somehow doesn't make me happy to bank on that.. > > > > Currently am doing bunch of initialization in .probe (platform driver) > > and I think recommendation would be to move that to .cra_init, which seem > > plausible but I don't have pdev to read hw_resource etc.. so would still > > need to get that. > > It seems that your allocation during probe relates to the hardware resource > where you only have one in the system. Thus, doing the allocation here makes > sense. And, you do not want to perform probe or such resource allocation once > per crypto API RNG instance allocation. As said, there can be multiple or even > they can be allocated and deallocated frequently. This in particular applies > if your driver's "stdrng" has the highest prio which means that it will be > allocated and deallocated frequently. Right, that is how I visualized it. Is there a way where we can tweak the register API to pass hw_resource pointer and get that back? Would that work with the security model in crypto. I do not like globals and somehow don't feel that we should do it that way Thanks for the quick look on the code. ~Vinod
Am Mittwoch, 27. Juni 2018, 09:01:44 CEST schrieb Vinod: Hi Vinod, > > > Currently am doing bunch of initialization in .probe (platform driver) > > > and I think recommendation would be to move that to .cra_init, which > > > seem > > > plausible but I don't have pdev to read hw_resource etc.. so would still > > > need to get that. > > > > It seems that your allocation during probe relates to the hardware > > resource > > where you only have one in the system. Thus, doing the allocation here > > makes sense. And, you do not want to perform probe or such resource > > allocation once per crypto API RNG instance allocation. As said, there > > can be multiple or even they can be allocated and deallocated frequently. > > This in particular applies if your driver's "stdrng" has the highest prio > > which means that it will be allocated and deallocated frequently. > > Right, that is how I visualized it. > > Is there a way where we can tweak the register API to pass hw_resource > pointer and get that back? Would that work with the security model in > crypto. I would not see an easy way for that. The core register API of the kernel crypto API would need to be changed. > > I do not like globals and somehow don't feel that we should do it that > way Granted, I concur here. Ciao Stephan
On Thu, Jun 21, 2018 at 6:53 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: > So does it generate one bit of output for each bit of hardware- > generated entropy like /dev/random? Or does it use a hardware- > generated seed to power a PRNG? I have some information to answer this question, although I'm not sure I can give a strict "yes/no" answer. There are a couple relevant documents: https://www.qualcomm.com/news/onq/2014/11/07/cryptographic-module-snapdragon-805-fips-140-2-certified https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp2944.pdf I also got response from a Qualcomm employee: "The Qualcomm random number generator used in Snapdragon chips consists of an entropy source coupled with the HASH-DRBG deterministic random bit generator from NIST Special Publication 800-90A, using SHA-256 as the hash function. The entropy source is based on sampled ring oscillators. Four ring oscillators are used to provide high assurance of adequate entropy. The entropy from the ring oscillators is conditioned using the 'derivation function' specified by NIST Special Publication 800-90A. The conditioned entropy is essentially perfect fully entropic data. It is used both to seed and to periodically reseed the DRGB." My understanding is that the PRNG is a real entropy source with some logic used to normalize the values. To quote: "No RNG uses data directly from the entropy source; bits in the output are likely correlated and unlikely to occur with 50% probability. The entropy post-processing is designed to turn dirty data in clean data." Based on the above, it seems to me that the Qualcomm PRNG qualifies as a real hardware RNG and porting to algif_rng is not the correct path.
On 28-06-18, 17:04, Timur Tabi wrote: > On Thu, Jun 21, 2018 at 6:53 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote: > > > So does it generate one bit of output for each bit of hardware- > > generated entropy like /dev/random? Or does it use a hardware- > > generated seed to power a PRNG? > > I have some information to answer this question, although I'm not sure > I can give a strict "yes/no" answer. > > There are a couple relevant documents: > > https://www.qualcomm.com/news/onq/2014/11/07/cryptographic-module-snapdragon-805-fips-140-2-certified > https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp2944.pdf > > I also got response from a Qualcomm employee: > > "The Qualcomm random number generator used in Snapdragon chips > consists of an entropy source coupled with the HASH-DRBG deterministic > random bit generator from NIST Special Publication 800-90A, using > SHA-256 as the hash function. > > The entropy source is based on sampled ring oscillators. Four ring > oscillators are used to provide high assurance of adequate entropy. > The entropy from the ring oscillators is conditioned using the > 'derivation function' specified by NIST Special Publication 800-90A. > The conditioned entropy is essentially perfect fully entropic data. > It is used both to seed and to periodically reseed the DRGB." > > My understanding is that the PRNG is a real entropy source with some > logic used to normalize the values. To quote: "No RNG uses data > directly from the entropy source; bits in the output are likely > correlated and unlikely to occur with 50% probability. The entropy > post-processing is designed to turn dirty data in clean data." > > Based on the above, it seems to me that the Qualcomm PRNG qualifies as > a real hardware RNG and porting to algif_rng is not the correct path. I think Stan did bring this point earlier that PRNG is compliant to FIPS-140-2. So it can be used by rng clients for various purposes but should not be fed to dev/random as the hw_random does. Herbert, can you please confirm..
On Fri, Jun 29, 2018 at 02:07:32PM +0530, Vinod wrote: > > I think Stan did bring this point earlier that PRNG is compliant to > FIPS-140-2. So it can be used by rng clients for various purposes but > should not be fed to dev/random as the hw_random does. > > Herbert, can you please confirm.. Yes this is a PRNG. A hardware RNG should return 1 bit of entropy per bit of output. Cheers,
diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c index 7644474035e5..3f509072a6c6 100644 --- a/drivers/char/hw_random/msm-rng.c +++ b/drivers/char/hw_random/msm-rng.c @@ -17,6 +17,7 @@ #include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> /* Device specific register offsets */ @@ -132,10 +133,16 @@ static struct hwrng msm_rng = { .read = msm_rng_read, }; +static struct hwrng msm_rng_v2 = { + .name = KBUILD_MODNAME, + .read = msm_rng_read, +}; + static int msm_rng_probe(struct platform_device *pdev) { struct resource *res; struct msm_rng *rng; + unsigned int version; int ret; rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL); @@ -154,6 +161,9 @@ static int msm_rng_probe(struct platform_device *pdev) return PTR_ERR(rng->clk); rng->hwrng = &msm_rng; + version = (unsigned long)of_device_get_match_data(&pdev->dev); + if (version) + rng->hwrng = &msm_rng_v2; rng->hwrng->priv = (unsigned long)rng; ret = devm_hwrng_register(&pdev->dev, rng->hwrng); @@ -166,7 +176,8 @@ static int msm_rng_probe(struct platform_device *pdev) } static const struct of_device_id msm_rng_of_match[] = { - { .compatible = "qcom,prng", }, + { .compatible = "qcom,prng", .data = (void *)0}, + { .compatible = "qcom,prng-v2", .data = (void *)1}, {} }; MODULE_DEVICE_TABLE(of, msm_rng_of_match);
Qcom 8996 and later chips support prng v2 where we need to only implement .read callback for hwrng. Add a new table for v2 which supports this and get version required for driver data. Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/char/hw_random/msm-rng.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)