Hello Michael,
> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Friday, November 12, 2021 10:18 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx>
> Cc: horia.geanta@xxxxxxx; pankaj.gupta@xxxxxxx;
> herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> iuliana.prodan@xxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxx/
-- andrey
