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

From: Michael Walle
Date: Fri Nov 05 2021 - 19:17:05 EST

Next message: Michael Walle: "Re: [PATCH v7 2/6] gpio: Add Delta TN48M CPLD GPIO driver"
Previous message: Laurent Pinchart: "Re: atomisp current issues"
In reply to: ZHIZHIKIN Andrey: "RE: [PATCH] crypto: caam - check jr permissions before probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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.

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

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

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

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.

That said, whats PRIM_TZ? When is it set?

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.

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

Next message: Michael Walle: "Re: [PATCH v7 2/6] gpio: Add Delta TN48M CPLD GPIO driver"
Previous message: Laurent Pinchart: "Re: atomisp current issues"
In reply to: ZHIZHIKIN Andrey: "RE: [PATCH] crypto: caam - check jr permissions before probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]