lkml.org 
[lkml]   [2021]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2 2/2] crypto: caam - check jr permissions before probing
Am 2021-11-11 17:46, schrieb Andrey Zhizhikin:
> 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.

> 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.

>
> Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
> ---
> 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_..

> +{
> + 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?

> + /*
> + * 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.

> + 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?

> +
> /*
> * 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.


> + 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.

> }
>
> - /* 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?

> 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.

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

\
 
 \ /
  Last update: 2021-11-12 22:19    [W:0.150 / U:0.572 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site