Message-ID: <AM6PR06MB4691A6F69B509EF6B4BFF466A68E9@AM6PR06MB4691.eurprd06.prod.outlook.com>
Date:   Fri, 5 Nov 2021 10:34:38 +0000
From:   ZHIZHIKIN Andrey <andrey.zhizhikin@...ca-geosystems.com>
To:     Michael Walle <michael@...le.cc>
CC:     "horia.geanta@....com" <horia.geanta@....com>,
        "pankaj.gupta@....com" <pankaj.gupta@....com>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "iuliana.prodan@....com" <iuliana.prodan@....com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] crypto: caam - check jr permissions before probing

Hello Michael,

> -----Original Message-----
> From: Michael Walle <michael@...le.cc>
> Sent: Friday, November 5, 2021 2:21 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@...ca-geosystems.com>
> Cc: horia.geanta@....com; pankaj.gupta@....com;
> herbert@...dor.apana.org.au; davem@...emloft.net;
> iuliana.prodan@....com; linux-crypto@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH] crypto: caam - check jr permissions before probing
> 
> 
> Hi Andrey,
> 
> Am 2021-11-04 17:21, 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. The only
> > condition that remains is: at least one Job Ring should be made
> > available for the NS world.
> 
> Where is that written down?

If you refer to the condition statement: this is implied as without any JR
initialized - it would not be possible to register a single algo.

This stemmed out from the discussion of the patch in U-Boot (see [1]),
where the it was indicated that JR0 is reserved for HAB on imx8m series.

The naïve solution proposed was to just disable the child node, but I
decided to look for a dynamic possibility to recognize those reserved JR
nodes, hence this patch came out.

> 
> > 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.
> >
> > Signed-off-by: Andrey Zhizhikin
> > <andrey.zhizhikin@...ca-geosystems.com>
> > ---
> >  drivers/crypto/caam/ctrl.c   | 18 ++++++++++++------
> >  drivers/crypto/caam/intern.h |  1 +
> >  drivers/crypto/caam/jr.c     | 17 +++++++++++++++++
> >  drivers/crypto/caam/regs.h   |  8 +++++---
> >  4 files changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index ca0361b2dbb0..c48506a02340 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -805,12 +805,18 @@ static int caam_probe(struct platform_device
> > *pdev)
> >       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
> > -                                          );
> > -                     ctrlpriv->total_jobrs++;
> > +                     /* Check if the JR is not owned exclusively by TZ */
> > +                     if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) &
> > +                             (MSTRID_LOCK_TZ_OWN |
> > + MSTRID_LOCK_PRIM_TZ))) {
> 
> what is the PRIM_TZ bit? I don't see it in my reference manual (which is for the
> LS1028A).

See my comment below regarding this bit.

> 
> Can't we just do a
> if (rd_reg32(&ctrl->jr_mid[ring].liodn_ms) &
>      (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ))
>         continue;

Yes, this would work as well. I'll address this is V2.

> 
> > +                             ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *)
> > +                                                  ((__force uint8_t *)ctrl +
> > +                                                  (ring + JR_BLOCK_NUMBER) *
> > +                                                   BLOCK_OFFSET
> > +                                                  );
> 
> This isn't really used at all. Can we drop "jr" from struct caam_drv_private
> altogether? See also below.

Agree. I was contemplating to remove this, as the caam_jr does its own devm_ioremap in the probe
and does not use what caam driver passes along. But I decided not to address this with this patch,
since this is not related to the issue this change addresses.

In general, this driver needs a bit of TLC, and I can take care of those points (together with
ctrlpriv->total_jobrs) in a separate series, unless there are strong objections arises that this cleanup
should come along this patch.

> 
> > +                             /* Indicate that this JR is available for NS */
> > +                             ctrlpriv->jobr_ns_flags |= BIT(ring);
> > +                             ctrlpriv->total_jobrs++;
> 
> as well as this?

Noted.

> 
> > +                     }
> >                       ring++;
> >               }
> >
> > diff --git a/drivers/crypto/caam/intern.h
> > b/drivers/crypto/caam/intern.h index 7d45b21bd55a..2919af55dbfe 100644
> > --- a/drivers/crypto/caam/intern.h
> > +++ b/drivers/crypto/caam/intern.h
> > @@ -91,6 +91,7 @@ struct caam_drv_private {
> >        * or from register-based version detection code
> >        */
> >       u8 total_jobrs;         /* Total Job Rings in device */
> > +     u8 jobr_ns_flags;       /* Flags for each Job Ring if it is not owned by
> > TZ*/
> >       u8 qi_present;          /* Nonzero if QI present in device */
> >       u8 mc_en;               /* Nonzero if MC f/w is active */
> >       int secvio_irq;         /* Security violation interrupt number */
> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index
> > 7f2b1101f567..a260981e0843 100644
> > --- a/drivers/crypto/caam/jr.c
> > +++ b/drivers/crypto/caam/jr.c
> > @@ -511,10 +511,27 @@ 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;
> > +     struct caam_drv_private *caamctrlpriv;
> >       static int total_jobrs;
> 
> ugh.

Yes, I've seen that. At first, I even wanted to replace it with the ctrlpriv->total_jobrs,
but decided not to do it in order to keep this patch focused.

> 
> >       struct resource *r;
> >       int error;
> >
> > +     /* Before we proceed with the JR device probing, verify
> > +      * that the job ring itself is available to Non-Secure world.
> > +      * If the JR is owned exclusively by TZ - we should not even
> > +      * process it further.
> > +      */
> > +     caamctrlpriv = dev_get_drvdata(pdev->dev.parent);
> > +     if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) {
> 
> Is anything preventing from total_jobrs getting big? Can't we get rid of this static
> variable somehow? Before this commit, it was just used for messages.

I guess not, the only limitation we have is the HW. Current imx8m mini does have
3 Job Rings, and plus added one more. Since I do not have any Layerscape HW -
I cannot really tell if the number of instantiated Job Rings there is bigger than 8
and would grow in the future.

I had a local implementation version with set_bit/test_bit variant, perhaps that
one would be more appropriate here. If it's OK - I can do that one and push it in V2.

> Can we check the TZ bit here instead of doing that bitflags dance?

Honestly, I had this implementation locally as well, but decided to go ahead with
"the dance" in order not to access the registers of another module from this one.
Besides, the JR node loop in present caam_probe() got me tripped, as it does an
early lookup and analysis of JR child nodes and I found it a right place to analyze
and record the JR availability.

> 
> nitpick: in caam there are no netdev comments. So multiline comments look like:
> /*
>   * this is a comment.
>   */

Noted, will be addressed in V2.

> 
> > +             /* This job ring is marked to be exclusively used by TZ,
> > +              * do not proceed with probing as the HW block is inaccessible.
> > +              * Increment total seen JR devices since it is used as the index
> > +              * into verification and fail probing for this node.
> > +              */
> > +             total_jobrs++;
> > +             return -ENODEV;
> > +     }
> > +
> >       jrdev = &pdev->dev;
> >       jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL);
> >       if (!jrpriv)
> > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> > index 3738625c0250..8ade617f9e65 100644
> > --- a/drivers/crypto/caam/regs.h
> > +++ b/drivers/crypto/caam/regs.h
> > @@ -445,10 +445,12 @@ 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_LOCK_PRIM_TZ          BIT(4)  /* only for JR: Primary TZ */
> 
> can't find that one.
> 
> in general, does these marcros match with your reference manual? Which one do
> you have?

I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit is defined in
JR[0:2]DID_MS registers.

The map looks like following:
LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], 
TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0]

Perhaps, there is a deviation here between what is instantiated in LS vs i.MX series.

At this point, I would also be interested to hear back from NXP on this.

> 
> for me the bits are named:
> LICID[31], AMTD[16], TZ[15] and SDID[11:0] also the register is called JRnICID.
> 
> I wonder if the LS1028A has a newer/older CAAM version.
> according to the device tree (fsl-ls1028a.dtsi) the sec is v5.0 (and also compatible
> with v4.0). If you have a look at the RM it says 7.0 (at least the MAJ_VER in
> SECVID_MS is 7 accoring to the manual; i haven't checked on the hardware for
> now.

I've checked the imx8m mini HW, and it has reported:
Major: 4
Minor: 1
Era: 9

I believe that the LS family has a newer version of CAAM instantiated, which can
be the reason on those register content deviations.

> 
> Horia, can you shed some light here.

+1

> 
> > -#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 */
> >
> > base-commit: 8a796a1dfca2780321755033a74bca2bbe651680
> 
> --
> -michael

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

-- andrey

Powered by blists - more mailing lists