[v3,2/5] crypto: caam: Fix endless loop when RNG is already initialized
diff mbox

Message ID 1517364040-27607-3-git-send-email-pure.logic@nexus-software.ie
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Bryan O'Donoghue Jan. 31, 2018, 2 a.m. UTC
commit 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
handles") introduces a control when incrementing ent_delay which contains
the following comment above it:

/*
 * If either SH were instantiated by somebody else
 * (e.g. u-boot) then it is assumed that the entropy
 * parameters are properly set and thus the function
 * setting these (kick_trng(...)) is skipped.
 * Also, if a handle was instantiated, do not change
 * the TRNG parameters.
 */

This is a problem observed when sec_init() has been run in u-boot and
and TrustZone is enabled. We can fix this by instantiating all rng state
handles in u-boot but, on the Kernel side we should ensure that this
non-terminating path is dealt with.

Fixes: 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
handles")

Reported-by: Ryan Harkin <ryan.harkin@linaro.org>
Cc: "Horia Geantă" <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
Cc: <stable@vger.kernel.org> # 4.12+
Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 drivers/crypto/caam/ctrl.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Horia Geanta Feb. 1, 2018, 12:16 p.m. UTC | #1
On 1/31/2018 4:00 AM, Bryan O'Donoghue wrote:
> commit 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
> handles") introduces a control when incrementing ent_delay which contains
> the following comment above it:
> 
> /*
>  * If either SH were instantiated by somebody else
>  * (e.g. u-boot) then it is assumed that the entropy
>  * parameters are properly set and thus the function
>  * setting these (kick_trng(...)) is skipped.
>  * Also, if a handle was instantiated, do not change
>  * the TRNG parameters.
>  */
> 
> This is a problem observed when sec_init() has been run in u-boot and
> and TrustZone is enabled. We can fix this by instantiating all rng state
> handles in u-boot but, on the Kernel side we should ensure that this
> non-terminating path is dealt with.
> 
> Fixes: 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
> handles")
> 
> Reported-by: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: "Horia Geantă" <horia.geanta@nxp.com>
> Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> Cc: <stable@vger.kernel.org> # 4.12+
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>  drivers/crypto/caam/ctrl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 98986d3..0a1e96b 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -704,7 +704,10 @@ static int caam_probe(struct platform_device *pdev)
>  					 ent_delay);
>  				kick_trng(pdev, ent_delay);
>  				ent_delay += 400;
> +			} else if (ctrlpriv->rng4_sh_init && inst_handles) {
> +				ent_delay += 400;
>  			}
If both RNG state handles are initialized before kernel runs, then
instantiate_rng() should be a no-op and return 0, which is enough to exit the
loop: while ((ret == -EAGAIN) && (ent_delay < RTSDCTL_ENT_DLY_MAX))

If the loop cannot exit based on value of "ret" != -EAGAIN, then it means
caam_probe() will eventually fail due to ret == -EAGAIN:
	if (ret) {
		dev_err(dev, "failed to instantiate RNG");
		goto caam_remove;
	}

Please provide more details, so that the root cause is found and fixed.
For e.g. what is the value of RDSTA (RNG DRNG Status register @0x6C0):
-before & after u-boot initializes RNG
-as seen by kernel during caam_probe()
Also provide related error messages displayed during boot.

Thanks,
Horia
Bryan O'Donoghue Feb. 2, 2018, 11:20 a.m. UTC | #2
On 01/02/18 12:16, Horia Geantă wrote:
> If the loop cannot exit based on value of "ret" != -EAGAIN, then it means
> caam_probe() will eventually fail due to ret == -EAGAIN:
> 	if (ret) {
> 		dev_err(dev, "failed to instantiate RNG");
> 		goto caam_remove;
> 	}

For me it's an endless loop applying the first two

https://patchwork.ozlabs.org/patch/866460/
https://patchwork.ozlabs.org/patch/866462/

but not this one

https://patchwork.ozlabs.org/patch/865890/

> Please provide more details, so that the root cause is found and fixed.

np

---
bod
Auer, Lukas Feb. 2, 2018, 12:54 p.m. UTC | #3
On Fri, 2018-02-02 at 11:20 +0000, Bryan O'Donoghue wrote:
> On 01/02/18 12:16, Horia Geantă wrote:

> > If the loop cannot exit based on value of "ret" != -EAGAIN, then it

> > means

> > caam_probe() will eventually fail due to ret == -EAGAIN:

> > 	if (ret) {

> > 		dev_err(dev, "failed to instantiate RNG");

> > 		goto caam_remove;

> > 	}

> 

> For me it's an endless loop applying the first two

> 

> https://patchwork.ozlabs.org/patch/866460/

> https://patchwork.ozlabs.org/patch/866462/

> 

> but not this one

> 

> https://patchwork.ozlabs.org/patch/865890/

> 

> > Please provide more details, so that the root cause is found and

> > fixed.

> 

> np

> 

> ---

> bod


I think the problem lies in the instantiate_rng() function. If the
driver is unable to acquire DEC0 it'll return -ENODEV. This should
terminate the while loop in the probe function. However, the return
value is never checked and is instead overwritten with -EAGAIN, causing
the endless loop.

This problem only occurs if u-boot instantiates only one of the state
handles (ent_delay doesn't get incremented) and the kernel runs in non-
secure mode (DEC0 can't get acquired). Instantiating all state handles
in u-boot therefore fixes this problem. In addition, the return value
in instantiate_rng() should be handled correctly by including

if (ret)
	break;

right after "ret = run_descriptor_deco0(ctrldev, desc, &status);".

Thanks,
Lukas
Horia Geanta Feb. 5, 2018, 8:45 a.m. UTC | #4
On 2/2/2018 2:54 PM, Auer, Lukas wrote:
> On Fri, 2018-02-02 at 11:20 +0000, Bryan O'Donoghue wrote:
>> On 01/02/18 12:16, Horia Geantă wrote:
>>> If the loop cannot exit based on value of "ret" != -EAGAIN, then it
>>> means
>>> caam_probe() will eventually fail due to ret == -EAGAIN:
>>> 	if (ret) {
>>> 		dev_err(dev, "failed to instantiate RNG");
>>> 		goto caam_remove;
>>> 	}
>>
>> For me it's an endless loop applying the first two
>>
>> https://patchwork.ozlabs.org/patch/866460/
>> https://patchwork.ozlabs.org/patch/866462/
>>
>> but not this one
>>
>> https://patchwork.ozlabs.org/patch/865890/
>>
[snip]
> 
> I think the problem lies in the instantiate_rng() function. If the
> driver is unable to acquire DEC0 it'll return -ENODEV. This should
> terminate the while loop in the probe function. However, the return
> value is never checked and is instead overwritten with -EAGAIN, causing
> the endless loop.
> 
> This problem only occurs if u-boot instantiates only one of the state
> handles (ent_delay doesn't get incremented) and the kernel runs in non-
> secure mode (DEC0 can't get acquired). Instantiating all state handles
> in u-boot therefore fixes this problem. In addition, the return value
> in instantiate_rng() should be handled correctly by including
> 
> if (ret)
> 	break;
> 
> right after "ret = run_descriptor_deco0(ctrldev, desc, &status);".
> 
Indeed, the error path is incorrect and should be fixed as you mentioned.
I will send a patch replacing this one.
Note that this fixes only the error path, meaning caam_probe() won't go into an
endless loop and instead will return -ENODEV, due to being unable to acquire
control of DECO0.

There are still a few hurdles to cross for CAAM to work in a TZ environment.

For e.g. could you please check / confirm whether DECO0MIDR (DECO0 MID registers
@0xA0, @0xA4) are set such that Linux kernel is allowed to r/w DECO0-related
registers?

Thanks,
Horia
Auer, Lukas Feb. 5, 2018, 1:54 p.m. UTC | #5
On Mon, 2018-02-05 at 08:45 +0000, Horia Geantă wrote:
> On 2/2/2018 2:54 PM, Auer, Lukas wrote:

> > On Fri, 2018-02-02 at 11:20 +0000, Bryan O'Donoghue wrote:

> > > On 01/02/18 12:16, Horia Geantă wrote:

> > > > If the loop cannot exit based on value of "ret" != -EAGAIN,

> > > > then it

> > > > means

> > > > caam_probe() will eventually fail due to ret == -EAGAIN:

> > > > 	if (ret) {

> > > > 		dev_err(dev, "failed to instantiate RNG");

> > > > 		goto caam_remove;

> > > > 	}

> > > 

> > > For me it's an endless loop applying the first two

> > > 

> > > https://patchwork.ozlabs.org/patch/866460/

> > > https://patchwork.ozlabs.org/patch/866462/

> > > 

> > > but not this one

> > > 

> > > https://patchwork.ozlabs.org/patch/865890/

> > > 

> 

> [snip]

> > 

> > I think the problem lies in the instantiate_rng() function. If the

> > driver is unable to acquire DEC0 it'll return -ENODEV. This should

> > terminate the while loop in the probe function. However, the return

> > value is never checked and is instead overwritten with -EAGAIN,

> > causing

> > the endless loop.

> > 

> > This problem only occurs if u-boot instantiates only one of the

> > state

> > handles (ent_delay doesn't get incremented) and the kernel runs in

> > non-

> > secure mode (DEC0 can't get acquired). Instantiating all state

> > handles

> > in u-boot therefore fixes this problem. In addition, the return

> > value

> > in instantiate_rng() should be handled correctly by including

> > 

> > if (ret)

> > 	break;

> > 

> > right after "ret = run_descriptor_deco0(ctrldev, desc, &status);".

> > 

> 

> Indeed, the error path is incorrect and should be fixed as you

> mentioned.

> I will send a patch replacing this one.

> Note that this fixes only the error path, meaning caam_probe() won't

> go into an

> endless loop and instead will return -ENODEV, due to being unable to

> acquire

> control of DECO0.

> 

> There are still a few hurdles to cross for CAAM to work in a TZ

> environment.

> 

> For e.g. could you please check / confirm whether DECO0MIDR (DECO0

> MID registers

> @0xA0, @0xA4) are set such that Linux kernel is allowed to r/w DECO0-

> related

> registers?

> 

> Thanks,

> Horia


On my board DECO0 MID ms is set to 0x8001, which I believe (going by
the structure of the other MID registers, since some of the bits are
only marked as reserved) is a MID of 1 (A7 cores) in secure mode.
Changing this to 0x9 for a MID of 1 in non-secure mode still fails the
DEC0 acquisition step in the probe call.

So unfortunately I am not sure what / if other steps are required to
use the CAAM in non-secure mode. Running a quick test with openssl
speed (using CAAM with cryptodev), it at least seems to be working.

Thanks,
Lukas

Patch
diff mbox

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 98986d3..0a1e96b 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -704,7 +704,10 @@  static int caam_probe(struct platform_device *pdev)
 					 ent_delay);
 				kick_trng(pdev, ent_delay);
 				ent_delay += 400;
+			} else if (ctrlpriv->rng4_sh_init && inst_handles) {
+				ent_delay += 400;
 			}
+
 			/*
 			 * if instantiate_rng(...) fails, the loop will rerun
 			 * and the kick_trng(...) function will modfiy the