Hello Michael,

-----Original Message-----
From: Michael Walle <redacted>
Sent: Friday, November 12, 2021 10:18 PM
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 v2 2/2] crypto: caam - check jr permissions before probing


Am 2021-11-11 17:46, 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.

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.

For other reviewers/maintainers: I'm still not sure this is the way to go. Instead
one can let u-boot fix up the device tree and remove or disable the JR node if its
not available.

Just as further clarification: this patch is intended to accommodate for cases where
JR is claimed in S world at the boot and not available for Kernel. It does not account
for fully dynamic cases, where JRs can be reclaimed between S <-> NS Worlds
during runtime. It rather accounts for situation when any arbitrary JR can be reserved
by any software entity before Kernel starts without a need to disable nodes at
compile time.

Full dynamic recognition is a part of much bigger task and is out of scope here.

quoted

If the Job Ring is released from the Secure World at the later stage,
then bind can be performed, which would check the Ring availability
and proceed with probing if the Ring is released.

But what if its the other way around and S world will "steal" it from NS world.

This is not accounted for in this patch, as I do not know if there is any
notification mechanism exists between S <-> NS world that would allow to
share the status of JR.

The implementation in this patch does provide a mechanism to perform a
later bind, but does not have any mechanism to indicate when it should be done...

quoted

Signed-off-by: Andrey Zhizhikin
[off-list ref]
---
Changes in V2:
- Create and export new method in CAAM control driver to verify JR
  validity based on the address supplied.
- Re-work the logic JR driver to call CAAM control method instead of
bit
  verification. This ensures the actual information retrival from CAAM
  module during JR probe.
- Clean-up of internal static job indexes used during probing, new
  indexing is performed based on the address supplied in DTB node.

 drivers/crypto/caam/ctrl.c   | 63 ++++++++++++++++++++++++++++++------
 drivers/crypto/caam/intern.h |  2 ++
 drivers/crypto/caam/jr.c     | 33 ++++++++++++++++---
 drivers/crypto/caam/regs.h   |  7 ++--
 4 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 7a14a69d89c7..ffe233f2c4ef 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c

@@ -79,6 +79,42 @@ static void build_deinstantiation_desc(u32 *desc,

int handle)
      append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT);  }

+/*
+ * caam_ctrl_check_jr_perm - check if the job ring can be accessed
+ *                           from Non-Secure World.
+ * @ctrldev - pointer to CAAM control device
+ * @ring_addr - input address of Job Ring, which access should be
verified
+ *
+ * Return: - 0 if Job Ring is available in NS World
+ *       - 1 if Job Ring is reserved in the Secure World
+ */
+inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32
ring_addr)

inline?
static int caam_ctrl_..

quoted

+{
+     struct caam_drv_private *ctrlpriv = dev_get_drvdata(ctrldev);
+     struct caam_ctrl __iomem *ctrl = ctrlpriv->ctrl;
+     u32 ring_id;
+
+     ring_id = ring_addr >>
+             ((ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) ?
+             16 : 12);

mh also here:
if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE)
   ring_id = ring_addr >> 16;
else
   ring_id = ring_addr >> 12;

Is there something to replace that "16" and "12" by the SZ_ macros, btw?

Good point, I'd need to look into this further on with what this can be replaced.

quoted

+     /*
+      * Check if the JR is not owned exclusively by TZ,
+      * and update capabilities bitmap to indicate that
+      * the Job Ring is available.
+      * Note: Ring index is 0-based in the register map
+      */
+     if (!((rd_reg32(&ctrl->jr_mid[ring_id - 1].liodn_ms)) &
+             MSTRID_LOCK_TZ_OWN)) {
+             ctrlpriv->caam_caps |= BIT(ring_id);

See also the former patch, this should be a macro.

True, would be covered in V3.

quoted

+             return 0;
+     }
+
+     /* Job Ring is reserved, clear bit if is was set before */
+     ctrlpriv->caam_caps &= ~BIT(ring_id);
+     return 1;
+}
+EXPORT_SYMBOL(caam_ctrl_check_jr_perm);

no need for exporting this, no?

Unfortunately, both CONFIG_CRYPTO_DEV_FSL_CAAM and
CONFIG_CRYPTO_DEV_FSL_CAAM_JR are tristate. Setting both
config options to "=m" fails to resolve caam_ctrl_check_jr_perm,
therefore I had to export it.

It strikes me odd however that CAAM can be compiled as module
without CAAM_JR module at all. This would imply that DECO is used
directly, which according to SRM is used for pure descriptor debug
purposes and should never be used in production.

I guess CRYPTO_DEV_FSL_CAAM _JR should be merged into
CRYPTO_DEV_FSL_CAAM, so they at least comes together. In that
case the export would not be necessary at all.

I must admit I didn't find this a good solution, therefore any advise
on a better solution here is highly appreciated.

quoted

+
 /*
  * run_descriptor_deco0 - runs a descriptor on DECO0, under direct
control of
  *                     the software (no JR/QI used).

@@ -612,7 +648,7 @@ static bool check_version(struct fsl_mc_version

*mc_version, u32 major,
 /* Probe routine for CAAM top (controller) level */  static int
caam_probe(struct platform_device *pdev)  {
-     int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
+     int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
      u64 caam_id;
      const struct soc_device_attribute *imx_soc_match;
      struct device *dev;

@@ -803,20 +839,27 @@ static int caam_probe(struct platform_device

*pdev)
 #endif
      }

-     ring = 0;
      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
-                                          );
-                     ring++;
-                     ctrlpriv->caam_caps |= BIT(ring);
+                     u32 ring_id;
+                     /*
+                      * Get register page to see the start address of CB
+                      * This is used to index into the bitmap of available
+                      * job rings and indicate if it is available in NS world.
+                      */
+                     ret = of_property_read_u32(np, "reg", &ring_id);

Not sure about this one, but I don't have any better idea.

Similar approach is proposed in U-Boot series [1].

quoted

+                     if (ret) {
+                             dev_err(dev, "failed to get register address for jobr\n");
+                             continue;
+                     }
+                     caam_ctrl_check_jr_perm(dev, ring_id);

What about
if (!caam_ctrl_is_jr_available(dev, ring_id))
    ctrlpriv->caam_caps &= ~BIT(ring_id);

Again BIT() should be its own macro.

Yes, would replace it in V3.

quoted

              }

-     /* If no QI and no rings specified, quit and go home */
+     /*
+      * If no QI, no rings specified or no rings available for NS -
+      * quit and go home
+      */
      if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) &&
          (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) ==
0)) {
              dev_err(dev, "no queues configured, terminating\n");

diff --git a/drivers/crypto/caam/intern.h

b/drivers/crypto/caam/intern.h index 37f0b93c7087..8d6a0cff556a 100644

--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h

@@ -115,6 +115,8 @@ struct caam_drv_private {  #endif  };

+inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32

ring_addr);
+
 #ifdef CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API

 int caam_algapi_init(struct device *dev); diff --git
a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index
7f2b1101f567..e1bc1ce5515e 100644

--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c

@@ -90,7 +90,7 @@ static int caam_reset_hw_jr(struct device *dev)

      if ((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) !=
          JRINT_ERR_HALT_COMPLETE || timeout == 0) {
-             dev_err(dev, "failed to flush job ring %d\n", jrp->ridx);
+             dev_err(dev, "failed to flush job ring %x\n",
+ jrp->ridx);

mh? why changing this?

After the change, jrp->ridx would contain JR hex address instead of index,
therefore I had to replace the debug output.

quoted

              return -EIO;
      }

@@ -101,7 +101,7 @@ static int caam_reset_hw_jr(struct device *dev)
              cpu_relax();

      if (timeout == 0) {
-             dev_err(dev, "failed to reset job ring %d\n", jrp->ridx);
+             dev_err(dev, "failed to reset job ring %x\n",
+ jrp->ridx);
              return -EIO;
      }

@@ -489,7 +489,7 @@ static int caam_jr_init(struct device *dev)
      error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt,

IRQF_SHARED,
                               dev_name(dev), dev);
      if (error) {
-             dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
+             dev_err(dev, "can't connect JobR %x interrupt (%d)\n",
                      jrp->ridx, jrp->irq);
              tasklet_kill(&jrp->irqtask);
      }

@@ -511,10 +511,33 @@ static int caam_jr_probe(struct platform_device

*pdev)
      struct device_node *nprop;
      struct caam_job_ring __iomem *ctrl;
      struct caam_drv_private_jr *jrpriv;
-     static int total_jobrs;
+     u32 ring_addr;
      struct resource *r;
      int error;

+     /*
+      * Get register page to see the start address of CB.
+      * Job Rings have their respective input base addresses
+      * defined in the register IRBAR_JRx. This address is
+      * present in the DT node and is aligned to the page
+      * size defined at CAAM compile time.
+      */
+     if (of_property_read_u32(pdev->dev.of_node, "reg", &ring_addr)) {
+             dev_err(&pdev->dev, "failed to get register address for jobr\n");
+             return -ENOMEM;
+     }
+
+     if (caam_ctrl_check_jr_perm(pdev->dev.parent, ring_addr)) {

With the change above, this will also have no bogus side effects on caam_caps.

quoted

+             /*
+              * This job ring is marked to be exclusively used by TZ,
+              * do not proceed with probing as the HW block is inaccessible.
+              * Defer this device probing for later, it might be released
+              * into NS world.
+              */
+             dev_info(&pdev->dev, "job ring is reserved in secure world\n");
+             return -ENODEV;
+     }
+
      jrdev = &pdev->dev;
      jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL);
      if (!jrpriv)

@@ -523,7 +546,7 @@ static int caam_jr_probe(struct platform_device

*pdev)
      dev_set_drvdata(jrdev, jrpriv);

      /* save ring identity relative to detection */
-     jrpriv->ridx = total_jobrs++;
+     jrpriv->ridx = ring_addr;

      nprop = pdev->dev.of_node;
      /* Get configuration properties from device tree */ diff --git
a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index
186e76e6a3e7..c4e8574bc570 100644

--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h

@@ -445,10 +445,11 @@ struct caam_perfmon {  };

 /* LIODN programming for DMA configuration */
-#define MSTRID_LOCK_LIODN    0x80000000
-#define MSTRID_LOCK_MAKETRUSTED      0x00010000      /* only for JR

masterid */

quoted

+#define MSTRID_LOCK_LIODN            BIT(31)
+#define MSTRID_LOCK_MAKETRUSTED      BIT(16) /* only for JR: masterid */
+#define MSTRID_LOCK_TZ_OWN           BIT(15) /* only for JR: owned by TZ */

-#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 */

--
-michael

Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211115070014.17586-2-gaurav.jain@nxp.com/

-- andrey