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 (3):
crypto: caam: Fix endless loop when RNG is already initialized
crypto: caam: add logic to detect when running under TrustZone
crypto: caam: detect RNG init when TrustZone is active
Rui Miguel Silva (3):
crypto: caam: Fix null dereference at error path
ARM: dts: imx7s: add CAAM device node
imx7d: add CAAM clocks
arch/arm/boot/dts/imx7s.dtsi | 26 +++++++++++++++++++
drivers/clk/imx/clk-imx7d.c | 3 +++
drivers/crypto/caam/ctrl.c | 45 ++++++++++++++++++++++++++++++---
drivers/crypto/caam/intern.h | 1 +
include/dt-bindings/clock/imx7d-clock.h | 5 +++-
5 files changed, 76 insertions(+), 4 deletions(-)
--
2.7.4
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.
*/
The problem is if u-boot has already instantiated the RNG and TEE/OPTEE is
running then instantiate_rng() will always return -EAGAIN and ent_delay
will never increment causing the loop to never break.
This patch fixes commit 1005bccd7a4a ("crypto: caam - enable instantiation
of all RNG4 state handles") by always incrementing ent_delay if and only if
both rng4_sh_init and inst_handles are non-zero for a given loop.
This should allow the current behavior to continue while allowing the loop
to break should (rng4_sh_init=1 && inst_handles 1) be true and
instantiate_rng() return -EAGAIN forever.
Fixes: 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
handles")
Reported-by: Ryan Harkin <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
---
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
This patch introduces logic to ascertain if the CAAM is running in
TrustZone mode or not. When running in TrustZone mode the first page of the
CAAM will read-back all zero for each register. This means for a register
such as the MCR - if we detect an all zero register - we can run a simple
test to try to toggle a bit inside of that register.
If the MCR is non-zero we already know we are in a non TrustZone mode.
If we read zero in the MCR but can successfully toggle a bit inside of the
MCR we know we are in a non TrustZone mode. So we set the bit back to zero
and continue.
If we read zero and cannot toggle a bit in the MCR we have successfully
detected TrustZone mode.
Once TrustZone is active the range of functions we can perform on CAAM is
limited; however the CAAM is still usable provided a previous stage in the
boot process initialized the block correctly.
Separate patches will handle the case of determining if the block is usable
when ctrlpriv->trustzone is true.
Signed-off-by: Bryan O'Donoghue <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
---
drivers/crypto/caam/ctrl.c | 21 +++++++++++++++++++++
drivers/crypto/caam/intern.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 0a1e96b..7fd3bfc 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -571,6 +571,27 @@ static int caam_probe(struct platform_device *pdev)
MCFGR_LONG_PTR : 0));
/*
+ * Detect if we are in TrustZone mode by trying to set MCFGR_LARGE_BURST
+ * In the first instance if TrustZone is active the MCR will read
+ * all-zero so if we read non-zero we know we can skip further checks.
+ * However its possible MCR is zero in non-TrustZone mode so if
+ * ctrl->mcr == 0 try to flip MCFGR_LARGE_BURST. If we cannot set the
+ * bit when MCR is zero we've detected TrustZone mode and then we know
+ * the first page of the CAAM is not accessible to Linux else flip
+ * MCFGR_LARGE_BURST back to off.
+ */
+ if (!rd_reg32(&ctrl->mcr)) {
+ clrsetbits_32(&ctrl->mcr, 0, MCFGR_LARGE_BURST);
+ if (!rd_reg32(&ctrl->mcr))
+ ctrlpriv->trust_zone = true;
+ else
+ clrsetbits_32(&ctrl->mcr, MCFGR_LARGE_BURST, 0);
+
+ if (ctrlpriv->trust_zone)
+ dev_info(dev, "TrustZone mode detected\n");
+ }
+
+ /*
* Read the Compile Time paramters and SCFGR to determine
* if Virtualization is enabled for this platform
*/
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index 91f1107..6ff382b 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -84,6 +84,7 @@ struct caam_drv_private {
u8 qi_present; /* Nonzero if QI present in device */
int secvio_irq; /* Security violation interrupt number */
int virt_en; /* Virtualization enabled in CAAM */
+ bool trust_zone; /* TrustZone mode detected */
#define RNG4_MAX_HANDLES 2
/* RNG4 block */
--
2.7.4
When TrustZone is enabled on sec4 compatible silicon the first page of the
CAAM is reserved for TrustZone only, this means that access to the deco
registers is restricted and will return zero when read.
The solution to this problem is to initialize the RNG prior to TrustZone
being enabled or to initialize the RNG from a TrustZone context and
simultaneously to ensure that the job-ring registers have been assigned to
the correct non-TrustZone context.
Assigning of the job-ring registers is a task for u-boot or OPTEE/TrustZone
as is the initialization of the RNG. This patch adds logic to detect RNG
initialization if and only if TrustZone has been detected as active on the
CAAM block.
If TrustZone is initialized and the RNG looks to be setup - we mark the RNG
as good to go and continue to load, else we mark the RNG as bad and bail
out.
More detail on the original problem and the split fix between u-boot and
Linux is available in these two threads
Link: https://github.com/OP-TEE/optee_os/issues/1408
Link: https://tinyurl.com/yam5gv9a
Link: https://patchwork.ozlabs.org/cover/865042
Signed-off-by: Bryan O'Donoghue <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
---
drivers/crypto/caam/ctrl.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 7fd3bfc..66a7c7e 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -711,6 +711,24 @@ static int caam_probe(struct platform_device *pdev)
int inst_handles =
rd_reg32(&ctrl->r4tst[0].rdsta) &
RDSTA_IFMASK;
+
+ /*
+ * If TrustZone is active then u-boot or the TrustZone
+ * firmware must have initialized the RNG for us else we
+ * cannot do so from Linux.
+ *
+ * We've previously detected TrustZone so now let's
+ * detect if the RNG has been initialized.
+ */
+ if (ctrlpriv->trust_zone) {
+ ret = -ENODEV;
+ if (ctrlpriv->rng4_sh_init || inst_handles)
+ ret = 0;
+ dev_info(dev, "TrustZone active RNG looks %s\n",
+ ret ? "uninitialized" : "initialized");
+ break;
+ }
+
/*
* If either SH were instantiated by somebody else
* (e.g. u-boot) then it is assumed that the entropy
--
2.7.4
From: Rui Miguel Silva <[email protected]>
Add CAAM clocks so that we could use the Cryptographic Acceleration and
Assurance Module (CAAM) hardware block.
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/clk/imx/clk-imx7d.c | 3 +++
include/dt-bindings/clock/imx7d-clock.h | 5 ++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 80dc211..fc4f0c4 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -795,6 +795,9 @@ 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_MEM_CLK] = imx_clk_gate4("caam_mem_clk", "ahb_root_clk", base + 0x4240, 0);
+ clks[IMX7D_CAAM_ACLK_CLK] = imx_clk_gate4("caam_aclk_clk", "ahb_root_clk", base + 0x4240, 0);
+ clks[IMX7D_CAAM_IPG_CLK] = imx_clk_gate4("caam_ipg_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..e17b59e 100644
--- a/include/dt-bindings/clock/imx7d-clock.h
+++ b/include/dt-bindings/clock/imx7d-clock.h
@@ -452,5 +452,8 @@
#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_MEM_CLK 442
+#define IMX7D_CAAM_ACLK_CLK 443
+#define IMX7D_CAAM_IPG_CLK 444
+#define IMX7D_CLK_END 445
#endif /* __DT_BINDINGS_CLOCK_IMX7D_H */
--
2.7.4
From: Rui Miguel Silva <[email protected]>
Add CAAM device node to the i.MX7s device tree.
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
arch/arm/boot/dts/imx7s.dtsi | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 82ad26e..0da146e 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -805,6 +805,32 @@
status = "disabled";
};
+ crypto: caam@30900000 {
+ compatible = "fsl,sec-v4.0";
+ fsl,sec-era = <4>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x30900000 0x10000>;
+ ranges = <0 0x30900000 0x10000>;
+ clocks = <&clks IMX7D_CAAM_MEM_CLK>,
+ <&clks IMX7D_CAAM_ACLK_CLK>,
+ <&clks IMX7D_CAAM_IPG_CLK>,
+ <&clks IMX7D_EIM_ROOT_CLK>;
+ clock-names = "mem", "aclk", "ipg", "emi_slow";
+
+ 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>;
+ };
+ };
+
flexcan1: can@30a00000 {
compatible = "fsl,imx7d-flexcan", "fsl,imx6q-flexcan";
reg = <0x30a00000 0x10000>;
--
2.7.4
From: Rui Miguel Silva <[email protected]>
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 <[email protected]>
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
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
On Wed, 2018-01-24 at 14:50 +0000, Bryan O'Donoghue wrote:
> From: Rui Miguel Silva <[email protected]>
>
> Add CAAM device node to the i.MX7s device tree.
>
> Signed-off-by: Rui Miguel Silva <[email protected]>
> Cc: "Horia Geantă" <[email protected]>
> Cc: Aymen Sghaier <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peng Fan <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Lukas Auer <[email protected]>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> arch/arm/boot/dts/imx7s.dtsi | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi
> b/arch/arm/boot/dts/imx7s.dtsi
> index 82ad26e..0da146e 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -805,6 +805,32 @@
> status = "disabled";
> };
>
> + crypto: caam@30900000 {
> + compatible = "fsl,sec-v4.0";
> + fsl,sec-era = <4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x30900000 0x10000>;
> + ranges = <0 0x30900000 0x10000>;
> + clocks = <&clks IMX7D_CAAM_MEM_CLK>,
> + <&clks
> IMX7D_CAAM_ACLK_CLK>,
> + <&clks IMX7D_CAAM_IPG_CLK>,
> + <&clks IMX7D_EIM_ROOT_CLK>;
> + clock-names = "mem", "aclk", "ipg",
> "emi_slow";
> +
> + 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>;
> + };
> + };
> +
> flexcan1: can@30a00000 {
> compatible = "fsl,imx7d-flexcan",
> "fsl,imx6q-flexcan";
> reg = <0x30a00000 0x10000>;
Looking at the device tree from the NXP kernel [1], the job ring and
clock configurations should be slightly different.
The job ring configuration has one additional job ring at offset 3000
and interrupt 114.
The clock configuration has changed to just one CAAM-specific clock in
addition to the ahb clock. This also means that additional
modifications to the CAAM driver are necessary or it will complain that
it doesn't find all clocks.
Thanks,
Lukas
[1] https://github.com/Freescale/linux-fslc/blob/4.1-2.0.x-imx/arch/arm
/boot/dts/imx7d.dtsi
On 24/01/18 18:12, Auer, Lukas wrote:
> On Wed, 2018-01-24 at 14:50 +0000, Bryan O'Donoghue wrote:
>> From: Rui Miguel Silva <[email protected]>
>>
>> Add CAAM device node to the i.MX7s device tree.
>>
>> Signed-off-by: Rui Miguel Silva <[email protected]>
>> Cc: "Horia Geantă" <[email protected]>
>> Cc: Aymen Sghaier <[email protected]>
>> Cc: Fabio Estevam <[email protected]>
>> Cc: Peng Fan <[email protected]>
>> Cc: Herbert Xu <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Lukas Auer <[email protected]>
>> Signed-off-by: Bryan O'Donoghue <[email protected]>
>> ---
>> arch/arm/boot/dts/imx7s.dtsi | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx7s.dtsi
>> b/arch/arm/boot/dts/imx7s.dtsi
>> index 82ad26e..0da146e 100644
>> --- a/arch/arm/boot/dts/imx7s.dtsi
>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>> @@ -805,6 +805,32 @@
>> status = "disabled";
>> };
>>
>> + crypto: caam@30900000 {
>> + compatible = "fsl,sec-v4.0";
>> + fsl,sec-era = <4>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + reg = <0x30900000 0x10000>;
>> + ranges = <0 0x30900000 0x10000>;
>> + clocks = <&clks IMX7D_CAAM_MEM_CLK>,
>> + <&clks
>> IMX7D_CAAM_ACLK_CLK>,
>> + <&clks IMX7D_CAAM_IPG_CLK>,
>> + <&clks IMX7D_EIM_ROOT_CLK>;
>> + clock-names = "mem", "aclk", "ipg",
>> "emi_slow";
>> +
>> + 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>;
>> + };
>> + };
>> +
>> flexcan1: can@30a00000 {
>> compatible = "fsl,imx7d-flexcan",
>> "fsl,imx6q-flexcan";
>> reg = <0x30a00000 0x10000>;
>
> Looking at the device tree from the NXP kernel [1], the job ring and
> clock configurations should be slightly different.
>
> The job ring configuration has one additional job ring at offset 3000
> and interrupt 114.
Yes true, that should be added.
We will do that in v2, thanks for spotting.
> The clock configuration has changed to just one CAAM-specific clock in
> addition to the ahb clock. This also means that additional
> modifications to the CAAM driver are necessary or it will complain that
> it doesn't find all clocks.
Sure - but, those clock changes aren't merged to upstream just yet, so
we won't those changes to this DTS addition yet.
Thanks for the review.
---
bod
On 24/01/18 22:48, Bryan O'Donoghue wrote:
>> The clock configuration has changed to just one CAAM-specific clock in
>> addition to the ahb clock. This also means that additional
>> modifications to the CAAM driver are necessary or it will complain that
>> it doesn't find all clocks.
>
> Sure - but, those clock changes aren't merged to upstream just yet, so
> we won't those changes to this DTS addition yet.
>
> Thanks for the review.
>
> ---
> bod
Ah Rui has pointed out to me for i.mx7 there are two clocks on on i.mx6
there are four clocks.
The bit that confused me there was the clock names on the 4.1 tree you
pointed to was clock-names = "caam_ipg", "caam_aclk"; and for 4.15 those
should be "ipg" and "aclk" by the looks of it.
So yes.
We will add the second job-ring and subtract the two extraneous clocks.
---
bod
On Wed, 2018-01-24 at 14:50 +0000, Bryan O'Donoghue wrote:
> When TrustZone is enabled on sec4 compatible silicon the first page
> of the
> CAAM is reserved for TrustZone only, this means that access to the
> deco
> registers is restricted and will return zero when read.
>
> The solution to this problem is to initialize the RNG prior to
> TrustZone
> being enabled or to initialize the RNG from a TrustZone context and
> simultaneously to ensure that the job-ring registers have been
> assigned to
> the correct non-TrustZone context.
>
> Assigning of the job-ring registers is a task for u-boot or
> OPTEE/TrustZone
> as is the initialization of the RNG. This patch adds logic to detect
> RNG
> initialization if and only if TrustZone has been detected as active
> on the
> CAAM block.
>
> If TrustZone is initialized and the RNG looks to be setup - we mark
> the RNG
> as good to go and continue to load, else we mark the RNG as bad and
> bail
> out.
>
> More detail on the original problem and the split fix between u-boot
> and
> Linux is available in these two threads
>
> Link: https://github.com/OP-TEE/optee_os/issues/1408
> Link: https://tinyurl.com/yam5gv9a
> Link: https://patchwork.ozlabs.org/cover/865042
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> Cc: "Horia Geantă" <[email protected]>
> Cc: Aymen Sghaier <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peng Fan <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Lukas Auer <[email protected]>
> ---
> drivers/crypto/caam/ctrl.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 7fd3bfc..66a7c7e 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -711,6 +711,24 @@ static int caam_probe(struct platform_device
> *pdev)
> int inst_handles =
> rd_reg32(&ctrl->r4tst[0].rdsta) &
> RDST
> A_IFMASK;
> +
> + /*
> + * If TrustZone is active then u-boot or the
> TrustZone
> + * firmware must have initialized the RNG
> for us else we
> + * cannot do so from Linux.
> + *
> + * We've previously detected TrustZone so
> now let's
> + * detect if the RNG has been initialized.
> + */
> + if (ctrlpriv->trust_zone) {
> + ret = -ENODEV;
> + if (ctrlpriv->rng4_sh_init ||
> inst_handles)
> + ret = 0;
> + dev_info(dev, "TrustZone active RNG
> looks %s\n",
> + ret ? "uninitialized" :
> "initialized");
> + break;
> + }
> +
> /*
> * If either SH were instantiated by
> somebody else
> * (e.g. u-boot) then it is assumed that the
> entropy
This (in addition to patch 5) should not be required if all RNG state
handles are already instantiated. The instantiate_rng() function checks
each state handle if it is already instantiated before trying to do so
itself. DEC0 would therefore never be used and the probe call should
succeed in non-secure mode.
I have submitted a patch [1] to u-boot that instantiates all RNG state
handles.
Thanks,
Lukas
[1] https://www.mail-archive.com/[email protected]/msg276184.html
On 1/24/2018 4:50 PM, Bryan O'Donoghue wrote:
> From: Rui Miguel Silva <[email protected]>
>
> 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")
Cc: <[email protected]> # 4.12+
>
> Tested-by: Ryan Harkin <[email protected]>
> Signed-off-by: Rui Miguel Silva <[email protected]>
> Cc: "Horia Geant?" <[email protected]>
> Cc: Aymen Sghaier <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peng Fan <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Lukas Auer <[email protected]>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
Reviewed-by: Horia Geant? <[email protected]>
Thanks,
Horia
On 1/24/2018 4:50 PM, Bryan O'Donoghue wrote:
> 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.
If the first ("global") caam register page is not accessible, RNG init is not
the only problem. For e.g. device endianness detection won't work. A complete
list could be generated by auditing all places where this page is r/w.
IMHO the correct direction for solving such cases (i.e. Linux kernel is provided
only with access to a few job rings) is to split the driver in two independent
ones - controller driver and job ring driver - and have corresponding DT nodes
for them. Controller DT node and one or more of the job ring DT nodes would be
deleted by the boot loader / trusted firmware if needed.
Of course, the job ring DT node might need additional properties for the driver
to work.
Thanks,
Horia
>
> 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 (3):
> crypto: caam: Fix endless loop when RNG is already initialized
> crypto: caam: add logic to detect when running under TrustZone
> crypto: caam: detect RNG init when TrustZone is active
>
> Rui Miguel Silva (3):
> crypto: caam: Fix null dereference at error path
> ARM: dts: imx7s: add CAAM device node
> imx7d: add CAAM clocks
>
> arch/arm/boot/dts/imx7s.dtsi | 26 +++++++++++++++++++
> drivers/clk/imx/clk-imx7d.c | 3 +++
> drivers/crypto/caam/ctrl.c | 45 ++++++++++++++++++++++++++++++---
> drivers/crypto/caam/intern.h | 1 +
> include/dt-bindings/clock/imx7d-clock.h | 5 +++-
> 5 files changed, 76 insertions(+), 4 deletions(-)
>
On 25/01/18 13:20, Auer, Lukas wrote:
> On Wed, 2018-01-24 at 14:50 +0000, Bryan O'Donoghue wrote:
>> When TrustZone is enabled on sec4 compatible silicon the first page
>> of the
>> CAAM is reserved for TrustZone only, this means that access to the
>> deco
>> registers is restricted and will return zero when read.
>>
>> The solution to this problem is to initialize the RNG prior to
>> TrustZone
>> being enabled or to initialize the RNG from a TrustZone context and
>> simultaneously to ensure that the job-ring registers have been
>> assigned to
>> the correct non-TrustZone context.
>>
>> Assigning of the job-ring registers is a task for u-boot or
>> OPTEE/TrustZone
>> as is the initialization of the RNG. This patch adds logic to detect
>> RNG
>> initialization if and only if TrustZone has been detected as active
>> on the
>> CAAM block.
>>
>> If TrustZone is initialized and the RNG looks to be setup - we mark
>> the RNG
>> as good to go and continue to load, else we mark the RNG as bad and
>> bail
>> out.
>>
>> More detail on the original problem and the split fix between u-boot
>> and
>> Linux is available in these two threads
>>
>> Link: https://github.com/OP-TEE/optee_os/issues/1408
>> Link: https://tinyurl.com/yam5gv9a
>> Link: https://patchwork.ozlabs.org/cover/865042
>>
>> Signed-off-by: Bryan O'Donoghue <[email protected]>
>> Cc: "Horia Geantă" <[email protected]>
>> Cc: Aymen Sghaier <[email protected]>
>> Cc: Fabio Estevam <[email protected]>
>> Cc: Peng Fan <[email protected]>
>> Cc: Herbert Xu <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Lukas Auer <[email protected]>
>> ---
>> drivers/crypto/caam/ctrl.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
>> index 7fd3bfc..66a7c7e 100644
>> --- a/drivers/crypto/caam/ctrl.c
>> +++ b/drivers/crypto/caam/ctrl.c
>> @@ -711,6 +711,24 @@ static int caam_probe(struct platform_device
>> *pdev)
>> int inst_handles =
>> rd_reg32(&ctrl->r4tst[0].rdsta) &
>> RDST
>> A_IFMASK;
>> +
>> + /*
>> + * If TrustZone is active then u-boot or the
>> TrustZone
>> + * firmware must have initialized the RNG
>> for us else we
>> + * cannot do so from Linux.
>> + *
>> + * We've previously detected TrustZone so
>> now let's
>> + * detect if the RNG has been initialized.
>> + */
>> + if (ctrlpriv->trust_zone) {
>> + ret = -ENODEV;
>> + if (ctrlpriv->rng4_sh_init ||
>> inst_handles)
>> + ret = 0;
>> + dev_info(dev, "TrustZone active RNG
>> looks %s\n",
>> + ret ? "uninitialized" :
>> "initialized");
>> + break;
>> + }
>> +
>> /*
>> * If either SH were instantiated by
>> somebody else
>> * (e.g. u-boot) then it is assumed that the
>> entropy
>
> This (in addition to patch 5) should not be required if all RNG state
> handles are already instantiated. The instantiate_rng() function checks
> each state handle if it is already instantiated before trying to do so
> itself. DEC0 would therefore never be used and the probe call should
> succeed in non-secure mode.
>
> I have submitted a patch [1] to u-boot that instantiates all RNG state
> handles.
>
> Thanks,
> Lukas
>
> [1] https://www.mail-archive.com/[email protected]/msg276184.html
>
Hi Lukas,
Yes that patch along with my patch to assign job-ring ownership looks
like it works.
https://www.mail-archive.com/[email protected]/msg275834.html
Provided both of those get in, we can drop these last two in this series
I think.
On 25/01/18 17:50, Horia Geant? wrote:
> If the first ("global") caam register page is not accessible, RNG init is not
> the only problem. For e.g. device endianness detection won't work.
Hi Horia,
Yes I had that thought that there were other gotchas lurking once the
CAAM was in a more restricted mode.
> A complete list could be generated by auditing all places where this page is r/w.
quote:
"The CAAM address space is divided into 16 4 KB pages to
match the access granularity of the MMU. Registers that are
intended to be accessed by a specific processor or process are
grouped into one of these 16 pages so that access to these
registers can be restricted via CAAM's own MID-based access
control mechanism, or via the CPU's MMU. For instance, the
general configuration and status registers are located within
page 0 and are intended to be accessed only by privileged
software. The registers that control each job ring are located in
a separate address block so that access to each job ring can be
restricted to a particular process. Some registers, such as the
version ID registers, are intended to be shared among processes.
Rather than require each CAAM driver process to have two
MMU page entries, one page for its private registers and one
for the shared registers, CAAM "aliases" these shared registers
into the upper section of each of the 16 address blocks. Reading
any one of the address aliases for the same register returns the
same information. Some of these aliased registers are writable,
so access to these registers may require that software implement
a concurrency control construct, as would be the case with any
register that is read/write accessible by multiple processes."
Not all of the first page is restricted - just some of it, I haven't
found a detailed explanation of which parts are restricted and which
parts are not.
Referring to "Security Reference Manual for i.MX 7Dual and 7Solo
Applications Processors, Rev. 0, 03/2017" section 8.10.1 you'll see
there are alias registers for example offset 0x204 DMA Control (DMAC)
which is r/w.
The above is the best description of the behavior I've found - I assume
there's some internal documentation (or RTL test benches) you guys can
refer to in NXP. I don't know if that's something you or Peng of Fabio
can follow up on internally.
> IMHO the correct direction for solving such cases (i.e. Linux kernel is provided
> only with access to a few job rings) is to split the driver in two independent
> ones - controller driver and job ring driver - and have corresponding DT nodes
> for them. Controller DT node and one or more of the job ring DT nodes would be
> deleted by the boot loader / trusted firmware if needed.
> Of course, the job ring DT node might need additional properties for the driver
> to work.
Well, I certainly take your point that there are probably other things
that are broken which we just haven't stumbled over yet. RNG broke for
us, we fixed it but, for example we haven't tried using RSA acceleration
so that might necessitate that type of change - or indeed what happens
if you make a call into the HAB (which will use the CAAM for crypto
acceleration) when the TZ restrictions are in-place.
Does HAB/ROM even run @ EL3 ? I expect so but, I don't know.
This series in V2 will just be about
1. Enabling for i.MX7
2. Fixing a few bugs we've found along the way.
It does look like we can solve the RNG permissions problem exclusively
in u-boot... but I will take some time to look at endianess and any
other low-hanging fruit you can suggest for that V2 series.
---
bod
Hi Rui,
I love your patch! Yet something to improve:
[auto build test ERROR on crypto/master]
[also build test ERROR on v4.15-rc9 next-20180126]
[cannot apply to cryptodev/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Bryan-O-Donoghue/Enable-CAAM-on-i-MX7s-fix-TrustZone-issues/20180127-185422
base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
config: arm-u8500_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: the linux-review/Bryan-O-Donoghue/Enable-CAAM-on-i-MX7s-fix-TrustZone-issues/20180127-185422 HEAD f907a172373d2c61dd7bf25a88621abc6e410f15 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/imx7s.dtsi:815.21-22 syntax error
FATAL ERROR: Unable to parse input tree
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi,
Thanks for the report.
On Sat 27 Jan 2018 at 15:49, kbuild test robot wrote:
> Hi Rui,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on crypto/master]
> [also build test ERROR on v4.15-rc9 next-20180126]
> [cannot apply to cryptodev/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Bryan-O-Donoghue/Enable-CAAM-on-i-MX7s-fix-TrustZone-issues/20180127-185422
> base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
> config: arm-u8500_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm
>
> Note: the linux-review/Bryan-O-Donoghue/Enable-CAAM-on-i-MX7s-fix-TrustZone-issues/20180127-185422 HEAD f907a172373d2c61dd7bf25a88621abc6e410f15 builds fine.
> It only hurts bisectibility.
Yeah, the order of the patches in the series were wrong and break
bisectibility.
V2 of this series already sent, besides other fixes, also fix this.
Once again many thanks.
---
Cheers,
Rui