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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux