diff mbox series

[v2,2/2] crypto: caam - check jr permissions before probing

Message ID 20211111164601.13135-3-andrey.zhizhikin@leica-geosystems.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series CAAM Driver: re-factor and dynamic JR availability check | expand

Commit Message

ZHIZHIKIN Andrey Nov. 11, 2021, 4:46 p.m. UTC
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.

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.

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(-)

Comments

Michael Walle Nov. 12, 2021, 9:17 p.m. UTC | #1
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 */
ZHIZHIKIN Andrey Nov. 15, 2021, 10:07 a.m. UTC | #2
Hello Michael,

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Friday, November 12, 2021 10:18 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> 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:
> > 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.

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

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

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

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

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

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

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

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

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

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

-- andrey
Michael Walle Nov. 15, 2021, 11:17 a.m. UTC | #3
>> > +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.

I see, and I'm too lazy at the moment to figure that out ;) but afaik
new exports should be only EXPORT_SYMBOL_GPL().

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

ahh then, ridx should renamed accordingly, I suppose.

-michael
Horia Geanta Nov. 18, 2021, 12:47 a.m. UTC | #4
On 11/15/2021 12:07 PM, ZHIZHIKIN Andrey wrote:
> Hello Michael,
> 
>> -----Original Message-----
>> From: Michael Walle <michael@walle.cc>
>> Sent: Friday, November 12, 2021 10:18 PM
>> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
>> 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:
>>> 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.
> 
I prefer f/w to fix the DT before passing it to the kernel,
either by adding the "secure-status" property (set explicitly to "disabled")
or by removing the job ring node(s) that are reserved.
OP-TEE already uses the first option. We should probably pick this up.

The reason I am supporting relying on DT and consequently avoiding registers
is that accessing page 0 in the caam register space from Non-secure world
should be avoided when caam is managed by Secure world (e.g. OP-TEE)
or a Secure Enclave (e.g. SECO).

Unfortunately support for HW-enforced access control for caam register space
is not that great / fine-grained, with the exception of more recent parts
like i.MX8MP and i.MX8ULP.

Horia
Michael Walle Nov. 18, 2021, 8:28 a.m. UTC | #5
Hi Horia,

>>>> 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.
>> 
> I prefer f/w to fix the DT before passing it to the kernel,
> either by adding the "secure-status" property (set explicitly to 
> "disabled")
> or by removing the job ring node(s) that are reserved.
> OP-TEE already uses the first option. We should probably pick this up.

Ah, nice:
Documentation/devicetree/bindings/arm/secure.txt

If I understand this correctly, if optee reserves a JR it will set the
secure-status to okay and status to disabled. (There is still a missing
link, how u-boot will then be passed this modified device tree, I might
miss something here.)

But what about the HAB, if I understand Andrey correct, then JR0 will
already be marked as "S world only" (or at least no EL3 program will
release it again). To me it looks like then either JR0 should be
(1) hardcoded to secure-status = "okay", status = "disabled", or (2)
u-boot SPL (or TF-A) should return it to NS world (and optee might
take it over again).

> The reason I am supporting relying on DT and consequently avoiding 
> registers
> is that accessing page 0 in the caam register space from Non-secure 
> world
> should be avoided when caam is managed by Secure world (e.g. OP-TEE)
> or a Secure Enclave (e.g. SECO).
> 
> Unfortunately support for HW-enforced access control for caam register 
> space
> is not that great / fine-grained, with the exception of more recent 
> parts
> like i.MX8MP and i.MX8ULP.

-michael
ZHIZHIKIN Andrey Nov. 18, 2021, 10:08 a.m. UTC | #6
Hello Horia/Michael,

I'd reply here to both of you since your answers are complementing each other.

I've also Cc: Gaurav here as he is working on CAAM support in U-Boot so this
discussion is relevant for him as well I suppose.

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Thursday, November 18, 2021 9:29 AM
> To: Horia Geantă <horia.geanta@nxp.com>
> Cc: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Pankaj Gupta
> <pankaj.gupta@nxp.com>; herbert@gondor.apana.org.au; davem@davemloft.net; Iuliana
> Prodan <iuliana.prodan@nxp.com>; linux-crypto@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing
> 
> 
> Hi Horia,
> 
> >>>> 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.
> >>
> > I prefer f/w to fix the DT before passing it to the kernel,
> > either by adding the "secure-status" property (set explicitly to
> > "disabled")
> > or by removing the job ring node(s) that are reserved.

According to the DT bindings doc mentioned by Michael below, it would not
be needed to remove the node.

Setting status = "disabled"; secure-status = "okay" should be enough to
reserve JR node in S World permanently. It would also serve the purpose
to indicate that the HW do provide the correct total amount of JRs, and
just some of then are not available to be used in NS World.

> > OP-TEE already uses the first option. We should probably pick this up.

Agree. I would drop the register access from this patch and follow-up with
DT node approach.

> 
> Ah, nice:
> Documentation/devicetree/bindings/arm/secure.txt

Good point, thanks for the doc guidance! This does provide a clear layout
on how the DT node should be crafted!

> 
> If I understand this correctly, if optee reserves a JR it will set the
> secure-status to okay and status to disabled. (There is still a missing
> link, how u-boot will then be passed this modified device tree, I might
> miss something here.)

I need to look at how OP-TEE does things here, but if they just set
secure-status = "okay" - then the JR should be visible in both worlds.

> 
> But what about the HAB, if I understand Andrey correct, then JR0 will
> already be marked as "S world only" (or at least no EL3 program will
> release it again).

It's a good point, which is still unclear: can JR0 be reclaimed back
after HAB is finished? Or should it stay in S-only world?

> To me it looks like then either JR0 should be
> (1) hardcoded to secure-status = "okay", status = "disabled", or (2)
> u-boot SPL (or TF-A) should return it to NS world (and optee might
> take it over again).

If the answer to HAB question is: it should stay in S World, then
I'd suggest to go with (1) as it presents the opportunity to define the
initial state of JR0 in deterministic state, without loosing the
information that HW does indeed have it implemented (node is present,
but permanently disabled). Later reclamation with this combination is
also possible.

What I would propose in addition here as well, is to define the
secure-status = "disabled" for all JR nodes on all derivatives, to have
the status set consistently. If later reclamation for any arbitrary JR
from NS to S world is needed - this property can be adjusted
accordingly by SW entity. Same thing should be done in U-Boot I suppose.

> 
> > The reason I am supporting relying on DT and consequently avoiding
> > registers
> > is that accessing page 0 in the caam register space from Non-secure
> > world
> > should be avoided when caam is managed by Secure world (e.g. OP-TEE)
> > or a Secure Enclave (e.g. SECO).

Understood, this is a valid point that I was missing. I'd re-work this
to use DT bindings and push it in V3.

I guess there would not be much left of this patch once I'd use DT approach,
so I'd re-spin the series to include DT bindings instead. JR driver clean-up
to remove static JR counter :| would go into the first one then.

> >
> > Unfortunately support for HW-enforced access control for caam register
> > space
> > is not that great / fine-grained, with the exception of more recent
> > parts
> > like i.MX8MP and i.MX8ULP.

Are there any particular distinct differences on those derivatives that
should be taken into account here with respect of JR reservation?
I might include those as well in this patch series if they are not that
significant, otherwise would try to address them separately.

> 
> -michael

Thanks to all of you for commenting here!

-- andrey
Michael Walle Nov. 18, 2021, 10:11 a.m. UTC | #7
Am 2021-11-18 11:08, schrieb ZHIZHIKIN Andrey:
> I guess there would not be much left of this patch once I'd use DT 
> approach,
> so I'd re-spin the series to include DT bindings instead. JR driver 
> clean-up
> to remove static JR counter :| would go into the first one then.

I really like the cleanup though! Esp. the removing of the static 
variable.

-michael
diff mbox series

Patch

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)
+{
+	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);
+	/*
+	 * 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);
+		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);
+
 /*
  * 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);
+			if (ret) {
+				dev_err(dev, "failed to get register address for jobr\n");
+				continue;
+			}
+			caam_ctrl_check_jr_perm(dev, ring_id);
 		}
 
-	/* 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);
 		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)) {
+		/*
+		 * 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 */