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

[ZHIZHIKIN Andrey]

Hello Michael,

> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Saturday, November 6, 2021 12:17 AM
> 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] crypto: caam - check jr permissions before probing
> 
> 
> Hi Andrey,
> 
> Am 2021-11-05 11:34, schrieb ZHIZHIKIN Andrey:
> >> -----Original Message-----
> >> From: Michael Walle <michael@xxxxxxxx>
> >> Sent: Friday, November 5, 2021 2:21 AM
> >> 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] 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.
> 
> First, thank you for taking the extra step here. Does "reserved for HAB"
> mean TF-A is using it or is the BootROM already using it. TF-A is optional (so is HAB
> I guess?). So it might be possible to have jr0 in linux, right? If that is correct, the
> solution to disable the jr0 at compile time is wrong.

From what I've seen in the U-Boot and ATF code, it seems that the JR0 is reserved
by BootROM. When the execution reaches the ATF (after SPL) those bits are already
set which concludes that the reservation is done quite early. Since current U-Boot
does not have any support for CAAM integrated yet (patchset is under review) -
it makes me believe that the reservation is done in BootROM.

It is correct though: if the JR is not reserved - then it is accessible in Linux, hence
compile-time disabling does not looked like a good solution to me.

> 
> Thus it has to be done at run time. Either by removing/disabling the node in u-
> boot or by not probing it. I'm not sure whats the correct solution here, though.

I was looking at various possibilities here (including OF_DYNAMIC), but could not
find any elegant solution that would cover this case so far. I'd continue to
experiment to see what would be the most appropriate here.

> 
> >> > 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@xxxxxxxxxxxxxxxxxxxx>
> >> > ---
> >> >  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.
> 
> If you clean up later, probably most of this code is going away, no?
> Then whats the point in having this patch in the first place. Usually its the other
> way around.

True, this is what I've realized once I looked at the implementation again.
I would include the clean-up and re-spin this patch as a series which would
contain it as well. Thanks for pointing it out!

> 
> >> > +                             /* 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.
> 
> Having the total_jobrs (and using it for anything else than messages) static will
> create an unnecessary dependency on the correct probe order.

Yes, I've stumbled upon this logical problem myself as well.

I'd decided that this should go, and would replace it with the option to use
IRBAR_JRx as the indexing, since it has the address aligned and can be used as a bit index.
- For LS1028A it would look like: IRBAR_JR[ring_id] >>  16
- For i.MX8M series it would be: IRBAR_JR[ring_id] >>  12

As those offsets are defined in the HW, they can be reliably used as indexing parameter
to interrogate the CAAM if the JR is reserved for TZ or not.

In addition, in order not to access the caam_ctrl register set from caam_jr driver, I'd still
prefer to use a bitmask and compare the bits set against that new indexing. This would
allow the driver to get rid of that static total_jobrs part at all.

I would appreciate the community opinion on the approach above whether it is plausible
and does not violate any kernel rules.

> 
> >> >       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.
> 
> For now the LS1028A has 4.

I had a look at the LS1028A SRM, and can confirm it does indeed have 4 JR.

> 
> > 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.
> 
> Ahh I didn't notice that the register was part of the parent. Meh.
> 
> > 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.
> 
> I see. But we should really communicate whether the child should return
> ENODEV in a different way. IMHO this static thing is really fragile.

If I go with the indexing option described above - this should be solved.

> 
> >>
> >> 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.
> 
> Probably, but that would also mean we'd have to distiguish between these too.
> You're checking PRIM_TZ which is SDID on the LS1028A (which is an arbitrary
> number if I understand it correctly). So the check above might actually trigger
> although it shouldn't.

It's maybe the opposite though: on the LS1028A if the TZ is set, then NS would
read SDID as all 0's. This presents the problem that PRIM_TZ bit defined for i.MX8M
series would read as 0 on LS series and fail the reservation check.

At this point I'd really like someone from NXP to comment on it, specifically: is it enough
to just check the TZ bit for all families to recognize that JR is reserved for usage in
Secure world only?

> 
> That said, whats PRIM_TZ? When is it set?

It is set together with TZ_OWN early at the boot and is used for several purposes, namely:
to derive SDID_MS (it is done dynamically), and also to indicate that the access to that JR
registers (config, interrupt, buffers, etc.) are only possible from Secure World.

> 
> >> 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.
> 
> probably, but as said above, we'd then need to distiguish between both. If you
> need to check PRIM_TZ which I haven't fully understood for now.

Correct, unless somebody from NXP could confirm that checking only TZ bit is
sufficient to understand that JR is reserved. In this case PRIM_TZ does not need
to be checked on i.MX8M series.

> 
> >
> >>
> >> 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@xxxxxxx/
> >
> > -- andrey
> 
> --
> -michael