RE: [PATCH] crypto: caam - check jr permissions before probing
From: ZHIZHIKIN Andrey <hidden>
Date: 2021-11-08 10:24:22
Also in:
lkml
Hello Michael,
-----Original Message----- From: Michael Walle <redacted> Sent: Saturday, November 6, 2021 12:17 AM To: ZHIZHIKIN Andrey <redacted> Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; herbert@gondor.apana.org.au; davem@davemloft.net; iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- kernel@vger.kernel.org Subject: Re: [PATCH] crypto: caam - check jr permissions before probing Hi Andrey, Am 2021-11-05 11:34, schrieb ZHIZHIKIN Andrey:quoted
quoted
-----Original Message----- From: Michael Walle <redacted> Sent: Friday, November 5, 2021 2:21 AM To: ZHIZHIKIN Andrey <redacted> Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; herbert@gondor.apana.org.au; davem@davemloft.net; iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- kernel@vger.kernel.org Subject: Re: [PATCH] crypto: caam - check jr permissions before probing Hi Andrey, Am 2021-11-04 17:21, schrieb Andrey Zhizhikin:quoted
Job Rings can be set to be exclusively used by TrustZone which makes the access to those rings only possible from Secure World. This access separation is defined by setting bits in CAAM JRxDID_MS register. Once reserved to be owned by TrustZone, this Job Ring becomes unavailable for the Kernel. This reservation is performed early in the boot process, even before the Kernel starts, which leads to unavailability of the HW at the probing stage. Moreover, the reservation can be done for any Job Ring and is not under control of the Kernel. The only condition that remains is: at least one Job Ring should be made available for the NS world.Where is that written down?If you refer to the condition statement: this is implied as without any JR initialized - it would not be possible to register a single algo. This stemmed out from the discussion of the patch in U-Boot (see [1]), where the it was indicated that JR0 is reserved for HAB on imx8m series. The naïve solution proposed was to just disable the child node, but I decided to look for a dynamic possibility to recognize those reserved JR nodes, hence this patch came out.First, thank you for taking the extra step here. Does "reserved for HAB" mean TF-A is using it or is the BootROM already using it. TF-A is optional (so is HAB I guess?). So it might be possible to have jr0 in linux, right? If that is correct, the solution to disable the jr0 at compile time is wrong.
From what I've seen in the U-Boot and ATF code, it seems that the JR0 is reserved by BootROM. When the execution reaches the ATF (after SPL) those bits are already set which concludes that the reservation is done quite early. Since current U-Boot does not have any support for CAAM integrated yet (patchset is under review) - it makes me believe that the reservation is done in BootROM. It is correct though: if the JR is not reserved - then it is accessible in Linux, hence compile-time disabling does not looked like a good solution to me.
Thus it has to be done at run time. Either by removing/disabling the node in u- boot or by not probing it. I'm not sure whats the correct solution here, though.
I was looking at various possibilities here (including OF_DYNAMIC), but could not find any elegant solution that would cover this case so far. I'd continue to experiment to see what would be the most appropriate here.
quoted
quoted
quoted
Current implementation lists Job Rings as child nodes of CAAM driver, and tries to perform probing on those regardless of whether JR HW is accessible or not. This leads to the following error while probing: [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 Implement a dynamic mechanism to identify which Job Ring is actually marked as owned by TrustZone, and fail the probing of those child nodes with -ENODEV. Signed-off-by: Andrey Zhizhikin [off-list ref] --- drivers/crypto/caam/ctrl.c | 18 ++++++++++++------ drivers/crypto/caam/intern.h | 1 + drivers/crypto/caam/jr.c | 17 +++++++++++++++++ drivers/crypto/caam/regs.h | 8 +++++--- 4 files changed, 35 insertions(+), 9 deletions(-)diff --git a/drivers/crypto/caam/ctrl.cb/drivers/crypto/caam/ctrl.c index ca0361b2dbb0..c48506a02340 100644--- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c@@ -805,12 +805,18 @@ static int caam_probe(struct platform_device*pdev) for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) - ((__force uint8_t *)ctrl + - (ring + JR_BLOCK_NUMBER) * - BLOCK_OFFSET - ); - ctrlpriv->total_jobrs++; + /* Check if the JR is not owned exclusively by TZ */ + if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) & + (MSTRID_LOCK_TZ_OWN | + MSTRID_LOCK_PRIM_TZ))) {what is the PRIM_TZ bit? I don't see it in my reference manual (which is for the LS1028A).See my comment below regarding this bit.quoted
Can't we just do a if (rd_reg32(&ctrl->jr_mid[ring].liodn_ms) & (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ)) continue;Yes, this would work as well. I'll address this is V2.quoted
quoted
+ ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force*)quoted
quoted
quoted
+ ((__force uint8_t *)ctrl + + (ring + JR_BLOCK_NUMBER) * + BLOCK_OFFSET + );This isn't really used at all. Can we drop "jr" from struct caam_drv_private altogether? See also below.Agree. I was contemplating to remove this, as the caam_jr does its own devm_ioremap in the probe and does not use what caam driver passes along. But I decided not to address this with this patch, since this is not related to the issue this change addresses. In general, this driver needs a bit of TLC, and I can take care of those points (together with ctrlpriv->total_jobrs) in a separate series, unless there are strong objections arises that this cleanup should come along this patch.If you clean up later, probably most of this code is going away, no? Then whats the point in having this patch in the first place. Usually its the other way around.
True, this is what I've realized once I looked at the implementation again. I would include the clean-up and re-spin this patch as a series which would contain it as well. Thanks for pointing it out!
quoted
quoted
quoted
+ /* Indicate that this JR is available for NS */ + ctrlpriv->jobr_ns_flags |= BIT(ring); + ctrlpriv->total_jobrs++;as well as this?Noted.quoted
quoted
+ } ring++; }diff --git a/drivers/crypto/caam/intern.hb/drivers/crypto/caam/intern.h index 7d45b21bd55a..2919af55dbfe 100644--- a/drivers/crypto/caam/intern.h +++ b/drivers/crypto/caam/intern.h@@ -91,6 +91,7 @@ struct caam_drv_private { * or from register-based version detection code */ u8 total_jobrs; /* Total Job Rings in device */ + u8 jobr_ns_flags; /* Flags for each Job Ring if it is not owned byTZ*/ u8 qi_present; /* Nonzero if QI present in device */ u8 mc_en; /* Nonzero if MC f/w is active */ int secvio_irq; /* Security violation interrupt number */diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.cindex 7f2b1101f567..a260981e0843 100644--- a/drivers/crypto/caam/jr.c +++ b/drivers/crypto/caam/jr.c@@ -511,10 +511,27 @@ static int caam_jr_probe(structplatform_device *pdev) struct device_node *nprop; struct caam_job_ring __iomem *ctrl; struct caam_drv_private_jr *jrpriv; + struct caam_drv_private *caamctrlpriv; static int total_jobrs;ugh.Yes, I've seen that. At first, I even wanted to replace it with the ctrlpriv->total_jobrs, but decided not to do it in order to keep this patch focused.Having the total_jobrs (and using it for anything else than messages) static will create an unnecessary dependency on the correct probe order.
Yes, I've stumbled upon this logical problem myself as well. I'd decided that this should go, and would replace it with the option to use IRBAR_JRx as the indexing, since it has the address aligned and can be used as a bit index. - For LS1028A it would look like: IRBAR_JR[ring_id] >> 16 - For i.MX8M series it would be: IRBAR_JR[ring_id] >> 12 As those offsets are defined in the HW, they can be reliably used as indexing parameter to interrogate the CAAM if the JR is reserved for TZ or not. In addition, in order not to access the caam_ctrl register set from caam_jr driver, I'd still prefer to use a bitmask and compare the bits set against that new indexing. This would allow the driver to get rid of that static total_jobrs part at all. I would appreciate the community opinion on the approach above whether it is plausible and does not violate any kernel rules.
quoted
quoted
quoted
struct resource *r; int error; + /* Before we proceed with the JR device probing, verify + * that the job ring itself is available to Non-Secure world. + * If the JR is owned exclusively by TZ - we should not even + * process it further. + */ + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); + if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) {Is anything preventing from total_jobrs getting big? Can't we get rid of this static variable somehow? Before this commit, it was just used for messages.I guess not, the only limitation we have is the HW. Current imx8m mini does have 3 Job Rings, and plus added one more. Since I do not have any Layerscape HW - I cannot really tell if the number of instantiated Job Rings there is bigger than 8 and would grow in the future.For now the LS1028A has 4.
I had a look at the LS1028A SRM, and can confirm it does indeed have 4 JR.
quoted
I had a local implementation version with set_bit/test_bit variant, perhaps that one would be more appropriate here. If it's OK - I can do that one and push it in V2.quoted
Can we check the TZ bit here instead of doing that bitflags dance?Honestly, I had this implementation locally as well, but decided to go ahead with "the dance" in order not to access the registers of another module from this one.Ahh I didn't notice that the register was part of the parent. Meh.quoted
Besides, the JR node loop in present caam_probe() got me tripped, as it does an early lookup and analysis of JR child nodes and I found it a right place to analyze and record the JR availability.I see. But we should really communicate whether the child should return ENODEV in a different way. IMHO this static thing is really fragile.
If I go with the indexing option described above - this should be solved.
quoted
quoted
nitpick: in caam there are no netdev comments. So multiline comments look like: /* * this is a comment. */Noted, will be addressed in V2.quoted
quoted
+ /* This job ring is marked to be exclusively used by TZ, + * do not proceed with probing as the HW block is inaccessible. + * Increment total seen JR devices since it is used as the index + * into verification and fail probing for this node. + */ + total_jobrs++; + return -ENODEV; + } + jrdev = &pdev->dev; jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); if (!jrpriv)diff --git a/drivers/crypto/caam/regs.hb/drivers/crypto/caam/regs.h index 3738625c0250..8ade617f9e65 100644--- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h@@ -445,10 +445,12 @@ struct caam_perfmon { }; /* LIODN programming for DMA configuration */ -#define MSTRID_LOCK_LIODN 0x80000000 -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JRmasterid */quoted
+#define MSTRID_LOCK_LIODN BIT(31) +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid*/quoted
quoted
quoted
+#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ*/quoted
quoted
quoted
+#define MSTRID_LOCK_PRIM_TZ BIT(4) /* only for JR: Primary TZ */can't find that one. in general, does these marcros match with your reference manual? Which one do you have?I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit is defined in JR[0:2]DID_MS registers. The map looks like following: LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] Perhaps, there is a deviation here between what is instantiated in LS vs i.MX series. At this point, I would also be interested to hear back from NXP on this.Probably, but that would also mean we'd have to distiguish between these too. You're checking PRIM_TZ which is SDID on the LS1028A (which is an arbitrary number if I understand it correctly). So the check above might actually trigger although it shouldn't.
It's maybe the opposite though: on the LS1028A if the TZ is set, then NS would read SDID as all 0's. This presents the problem that PRIM_TZ bit defined for i.MX8M series would read as 0 on LS series and fail the reservation check. At this point I'd really like someone from NXP to comment on it, specifically: is it enough to just check the TZ bit for all families to recognize that JR is reserved for usage in Secure world only?
That said, whats PRIM_TZ? When is it set?
It is set together with TZ_OWN early at the boot and is used for several purposes, namely: to derive SDID_MS (it is done dynamically), and also to indicate that the access to that JR registers (config, interrupt, buffers, etc.) are only possible from Secure World.
quoted
quoted
for me the bits are named: LICID[31], AMTD[16], TZ[15] and SDID[11:0] also the register is called JRnICID. I wonder if the LS1028A has a newer/older CAAM version. according to the device tree (fsl-ls1028a.dtsi) the sec is v5.0 (and also compatible with v4.0). If you have a look at the RM it says 7.0 (at least the MAJ_VER in SECVID_MS is 7 accoring to the manual; i haven't checked on the hardware for now.I've checked the imx8m mini HW, and it has reported: Major: 4 Minor: 1 Era: 9 I believe that the LS family has a newer version of CAAM instantiated, which can be the reason on those register content deviations.probably, but as said above, we'd then need to distiguish between both. If you need to check PRIM_TZ which I haven't fully understood for now.
Correct, unless somebody from NXP could confirm that checking only TZ bit is sufficient to understand that JR is reserved. In this case PRIM_TZ does not need to be checked on i.MX8M series.
quoted
quoted
Horia, can you shed some light here.+1quoted
quoted
-#define MSTRID_LIODN_MASK 0x0fff +#define MSTRID_LIODN_MASK GENMASK(11, 0) struct masterid { u32 liodn_ms; /* lock and make-trusted control bits */ u32 liodn_ls; /* LIODN for non-sequence and seq access */ base-commit: 8a796a1dfca2780321755033a74bca2bbe651680-- -michaelLink: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211026065554.29009-4-gaurav.jain@nxp.com/ -- andrey-- -michael