[PATCH v3 0/5] Enable CAAM on i.MX7s fix TrustZone issues

[PATCH v3 0/5] Enable CAAM on i.MX7s fix TrustZone issues

[Bryan O'Donoghue]

V3:
- Added Cc: clk driver maintainers - Fabio Estevam
- Added Cc: i.MX arch maintainers - Fabio Estevam
- Removed bouncing email address for Herbert Xu

V2-resend:
- Patch 0005 lost in the ether - resending

V2:
- Endian detection is ok with TrustZone enabled Horia.
  Endian detection logic tested with TrustZone enabled. The register that
  this relies on though isn't affected by the lock-down in the first page.
  Assuming set of affected registers is actually just the 'deco' registers
  though there is no formal statement of that, that I am aware of.

- Moving of TrustZone work-around into u-boot
  This set actually doesn't need to deal with TrustZone at all now but, for
  the sake of consistency keeping thread title

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

- Reworded endless loop fix to read a bit better

- Fixes to DTS additions - Rui

- Fixes to number of clocks declared - Rui

V1:
This patch-set enables CAAM on the i.MX7s and fixes a number of issues
identified with the CAAM driver and hardware when TrustZone mode is
enabled.

The first block of patches are simple bug-fixes, followed by a second block
of patches which are simple enabling patches for the i.MX7Solo - note we
aren't enabling for the i.MX7Dual since we don't have hardware to test that
out but it should be a 1:1 mapping for others to enable when appropriate.

The final block in this series implements a fix for using the CAAM when
OPTEE/TrustZone is enabled. The various details are logged in these
threads.

Link: https://github.com/OP-TEE/optee_os/issues/1408
Link: https://tinyurl.com/yam5gv9a
Link: https://patchwork.ozlabs.org/cover/865042

In simple terms, when TrustZone is active the first page of the CAAM
becomes inaccessible to Linux as it has a special 'TZ bit' associated with
it that software cannot toggle or even view AFAIK.

The patches here then

1. Detect when TrustZone is active
2. Detect if u-boot (or OPTEE) has already initialized the RNG

and loads the CAAM driver in a different way - skipping over the RNG
initialization that Linux now no-longer has permissions to carry out.

Should #1 be true but #2 not be true, driver loading stops (and Rui's patch
for the NULL pointer dereference fixes a cash on this path). If #2 is true
but #1 is not then it's a NOP as Linux has full permission to rewrite the
deco registers in the first page of CAAM registers.

Finally then if #1 and #2 are true, the fixes here allow the CAAM to come
up and for the RNG to be useable again.

Bryan O'Donoghue (1):
  crypto: caam: Fix endless loop when RNG is already initialized

Rui Miguel Silva (4):
  crypto: caam: Fix null dereference at error path
  crypto: caam: do not use mem and emi_slow clock for imx7x
  clk: imx7d: add CAAM clock
  ARM: dts: imx7s: add CAAM device node

 arch/arm/boot/dts/imx7s.dtsi            | 31 +++++++++++++++++++++++
 drivers/clk/imx/clk-imx7d.c             |  1 +
 drivers/crypto/caam/ctrl.c              | 45 ++++++++++++++++++++-------------
 include/dt-bindings/clock/imx7d-clock.h |  3 ++-
 4 files changed, 61 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH v3 1/5] crypto: caam: Fix null dereference at error path

[Bryan O'Donoghue]

From: Rui Miguel Silva <rui.silva@linaro.org>

caam_remove already removes the debugfs entry, so we need to remove the one
immediately before calling caam_remove.

This fix a NULL dereference at error paths is caam_probe fail.

[bod: changed name prefix to "crypto: caam: Fix .."]
[bod: added Fixes tag]

Fixes: 67c2315def06 ("crypto: caam - add Queue Interface (QI) backend
support")

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
Signed-off-by: Rui Miguel Silva <rui.silva@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+
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 drivers/crypto/caam/ctrl.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 027e121..98986d3 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -809,9 +809,6 @@ static int caam_probe(struct platform_device *pdev)
 	return 0;
 
 caam_remove:
-#ifdef CONFIG_DEBUG_FS
-	debugfs_remove_recursive(ctrlpriv->dfs_root);
-#endif
 	caam_remove(pdev);
 	return ret;
 
-- 
2.7.4

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

[Bryan O'Donoghue]

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 instantiate_rng(...) fails, the loop will rerun
 			 * and the kick_trng(...) function will modfiy the
-- 
2.7.4

[PATCH v3 3/5] crypto: caam: do not use mem and emi_slow clock for imx7x

[Bryan O'Donoghue]

From: Rui Miguel Silva <rui.silva@linaro.org>

I.MX7x only use two clocks for the CAAM module, so make sure we do not try to
use the mem and the emi_slow clock when running in that imx7d and imx7s machine
type.

[bod: fixed minor trailing whitespace issue]

Signed-off-by: Rui Miguel Silva <rui.silva@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>
Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 drivers/crypto/caam/ctrl.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 0a1e96b..1f2dd6a 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -333,7 +333,8 @@ static int caam_remove(struct platform_device *pdev)
 
 	/* shut clocks off before finalizing shutdown */
 	clk_disable_unprepare(ctrlpriv->caam_ipg);
-	clk_disable_unprepare(ctrlpriv->caam_mem);
+	if (ctrlpriv->caam_mem)
+		clk_disable_unprepare(ctrlpriv->caam_mem);
 	clk_disable_unprepare(ctrlpriv->caam_aclk);
 	if (ctrlpriv->caam_emi_slow)
 		clk_disable_unprepare(ctrlpriv->caam_emi_slow);
@@ -462,14 +463,17 @@ static int caam_probe(struct platform_device *pdev)
 	}
 	ctrlpriv->caam_ipg = clk;
 
-	clk = caam_drv_identify_clk(&pdev->dev, "mem");
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		dev_err(&pdev->dev,
-			"can't identify CAAM mem clk: %d\n", ret);
-		return ret;
+	if (!of_machine_is_compatible("fsl,imx7d") &&
+	    !of_machine_is_compatible("fsl,imx7s")) {
+		clk = caam_drv_identify_clk(&pdev->dev, "mem");
+		if (IS_ERR(clk)) {
+			ret = PTR_ERR(clk);
+			dev_err(&pdev->dev,
+				"can't identify CAAM mem clk: %d\n", ret);
+			return ret;
+		}
+		ctrlpriv->caam_mem = clk;
 	}
-	ctrlpriv->caam_mem = clk;
 
 	clk = caam_drv_identify_clk(&pdev->dev, "aclk");
 	if (IS_ERR(clk)) {
@@ -480,7 +484,9 @@ static int caam_probe(struct platform_device *pdev)
 	}
 	ctrlpriv->caam_aclk = clk;
 
-	if (!of_machine_is_compatible("fsl,imx6ul")) {
+	if (!of_machine_is_compatible("fsl,imx6ul") &&
+	    !of_machine_is_compatible("fsl,imx7d") &&
+	    !of_machine_is_compatible("fsl,imx7s")) {
 		clk = caam_drv_identify_clk(&pdev->dev, "emi_slow");
 		if (IS_ERR(clk)) {
 			ret = PTR_ERR(clk);
@@ -497,11 +503,13 @@ static int caam_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(ctrlpriv->caam_mem);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "can't enable CAAM secure mem clock: %d\n",
-			ret);
-		goto disable_caam_ipg;
+	if (ctrlpriv->caam_mem) {
+		ret = clk_prepare_enable(ctrlpriv->caam_mem);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "can't enable CAAM secure mem clock: %d\n",
+				ret);
+			goto disable_caam_ipg;
+		}
 	}
 
 	ret = clk_prepare_enable(ctrlpriv->caam_aclk);
@@ -823,7 +831,8 @@ static int caam_probe(struct platform_device *pdev)
 disable_caam_aclk:
 	clk_disable_unprepare(ctrlpriv->caam_aclk);
 disable_caam_mem:
-	clk_disable_unprepare(ctrlpriv->caam_mem);
+	if (ctrlpriv->caam_mem)
+		clk_disable_unprepare(ctrlpriv->caam_mem);
 disable_caam_ipg:
 	clk_disable_unprepare(ctrlpriv->caam_ipg);
 	return ret;
-- 
2.7.4

[PATCH v3 4/5] clk: imx7d: add CAAM clock

[Bryan O'Donoghue]

From: Rui Miguel Silva <rui.silva@linaro.org>

Add CAAM clock so that we could use the Cryptographic Acceleration and
Assurance Module (CAAM) hardware block.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.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>
Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 drivers/clk/imx/clk-imx7d.c             | 1 +
 include/dt-bindings/clock/imx7d-clock.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 80dc211..52ab096 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -795,6 +795,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] = imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base + 0x4130, 0);
 	clks[IMX7D_DRAM_ALT_ROOT_CLK] = imx_clk_gate4("dram_alt_root_clk", "dram_alt_post_div", base + 0x4130, 0);
 	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk", "ipg_root_clk", base + 0x4230, 0);
+	clks[IMX7D_CAAM_CLK] = imx_clk_gate4("caam_clk", "ipg_root_clk", base + 0x4240, 0);
 	clks[IMX7D_USB_HSIC_ROOT_CLK] = imx_clk_gate4("usb_hsic_root_clk", "usb_hsic_post_div", base + 0x4420, 0);
 	clks[IMX7D_SDMA_CORE_CLK] = imx_clk_gate4("sdma_root_clk", "ahb_root_clk", base + 0x4480, 0);
 	clks[IMX7D_PCIE_CTRL_ROOT_CLK] = imx_clk_gate4("pcie_ctrl_root_clk", "pcie_ctrl_post_div", base + 0x4600, 0);
diff --git a/include/dt-bindings/clock/imx7d-clock.h b/include/dt-bindings/clock/imx7d-clock.h
index e2f99ae..2bc5618 100644
--- a/include/dt-bindings/clock/imx7d-clock.h
+++ b/include/dt-bindings/clock/imx7d-clock.h
@@ -452,5 +452,6 @@
 #define IMX7D_OCOTP_CLK			439
 #define IMX7D_NAND_RAWNAND_CLK		440
 #define IMX7D_NAND_USDHC_BUS_RAWNAND_CLK 441
-#define IMX7D_CLK_END			442
+#define IMX7D_CAAM_CLK			442
+#define IMX7D_CLK_END			443
 #endif /* __DT_BINDINGS_CLOCK_IMX7D_H */
-- 
2.7.4

[PATCH v3 5/5] ARM: dts: imx7s: add CAAM device node

[Bryan O'Donoghue]

From: Rui Miguel Silva <rui.silva@linaro.org>

Add CAAM device node to the i.MX7s device tree.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.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>
Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 82ad26e..e38c159 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -805,6 +805,37 @@
 				status = "disabled";
 			};
 
+			crypto: caam@30900000 {
+				compatible = "fsl,sec-v4.0";
+				fsl,sec-era = <4>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x30900000 0x40000>;
+				ranges = <0 0x30900000 0x40000>;
+				interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_CAAM_CLK>,
+					 <&clks IMX7D_AHB_CHANNEL_ROOT_CLK>;
+				clock-names = "ipg", "aclk";
+
+				sec_jr0: jr0@1000 {
+					compatible = "fsl,sec-v4.0-job-ring";
+					reg = <0x1000 0x1000>;
+					interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+				};
+
+				sec_jr1: jr1@2000 {
+					compatible = "fsl,sec-v4.0-job-ring";
+					reg = <0x2000 0x1000>;
+					interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+				};
+
+				sec_jr2: jr1@3000 {
+					compatible = "fsl,sec-v4.0-job-ring";
+					reg = <0x3000 0x1000>;
+					interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>;
+				};
+			};
+
 			flexcan1: can@30a00000 {
 				compatible = "fsl,imx7d-flexcan", "fsl,imx6q-flexcan";
 				reg = <0x30a00000 0x10000>;
-- 
2.7.4

Re: [PATCH v3 5/5] ARM: dts: imx7s: add CAAM device node

[Horia Geantă]

On 1/31/2018 4:00 AM, Bryan O'Donoghue wrote:
> From: Rui Miguel Silva <rui.silva@linaro.org>
> 
> Add CAAM device node to the i.MX7s device tree.
> 
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.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>
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>  arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 82ad26e..e38c159 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -805,6 +805,37 @@
>  				status = "disabled";
>  			};
>  
> +			crypto: caam@30900000 {
> +				compatible = "fsl,sec-v4.0";
> +				fsl,sec-era = <4>;
CCBVID[CAAM_ERA] = 8.
Either remove this (optional) property and let the bootloader add it
dynamically, or provide a correct value for it.

Thanks,
Horia

Re: [PATCH v3 2/5] crypto: caam: Fix endless loop when RNG is already initialized

[Horia Geantă]

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

Re: [PATCH v3 4/5] clk: imx7d: add CAAM clock

[Fabio Estevam]

On Wed, Jan 31, 2018 at 12:00 AM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> From: Rui Miguel Silva <rui.silva@linaro.org>
>
> Add CAAM clock so that we could use the Cryptographic Acceleration and
> Assurance Module (CAAM) hardware block.
>
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: linux-clk@vger.kernel.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>
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH v3 2/5] crypto: caam: Fix endless loop when RNG is already initialized

[Bryan O'Donoghue]

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

Re: [PATCH v3 2/5] crypto: caam: Fix endless loop when RNG is already initialized

[Auer, Lukas]

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

Re: [PATCH v3 2/5] crypto: caam: Fix endless loop when RNG is already initialized

[Horia Geantă]

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

[PATCH] crypto: caam - fix endless loop when DECO acquire fails

[Horia Geantă]

In case DECO0 cannot be acquired - i.e. run_descriptor_deco0() fails
with -ENODEV, caam_probe() enters an endless loop:

run_descriptor_deco0
	ret -ENODEV
	-> instantiate_rng
		-ENODEV, overwritten by -EAGAIN
		ret -EAGAIN
		-> caam_probe
			-EAGAIN results in endless loop

It turns out the error path in instantiate_rng() is incorrect,
the checks are done in the wrong order.

Cc: <stable@vger.kernel.org> # 3.13+
Fixes: 1005bccd7a4a6 ("crypto: caam - enable instantiation of all RNG4 state handles")
Reported-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Suggested-by: Auer Lukas <lukas.auer@aisec.fraunhofer.de>
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
 drivers/crypto/caam/ctrl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 75d280cb2dc0..e843cf410373 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -228,12 +228,16 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
 		 * without any error (HW optimizations for later
 		 * CAAM eras), then try again.
 		 */
+		if (ret)
+			break;
+
 		rdsta_val = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
 		if ((status && status != JRSTA_SSRC_JUMP_HALT_CC) ||
-		    !(rdsta_val & (1 << sh_idx)))
+		    !(rdsta_val & (1 << sh_idx))) {
 			ret = -EAGAIN;
-		if (ret)
 			break;
+		}
+
 		dev_info(ctrldev, "Instantiated RNG4 SH%d\n", sh_idx);
 		/* Clear the contents before recreating the descriptor */
 		memset(desc, 0x00, CAAM_CMD_SZ * 7);
-- 
2.12.0.264.gd6db3f216544

Re: [PATCH v3 2/5] crypto: caam: Fix endless loop when RNG is already initialized

[Auer, Lukas]

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