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

[ZHIZHIKIN Andrey]

Hello Horia/Michael,

I'd reply here to both of you since your answers are complementing each other.

I've also Cc: Gaurav here as he is working on CAAM support in U-Boot so this
discussion is relevant for him as well I suppose.

> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Thursday, November 18, 2021 9:29 AM
> To: Horia Geantă <horia.geanta@xxxxxxx>
> Cc: ZHIZHIKIN Andrey <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx>; Pankaj Gupta
> <pankaj.gupta@xxxxxxx>; herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; Iuliana
> Prodan <iuliana.prodan@xxxxxxx>; linux-crypto@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing
> 
> 
> Hi Horia,
> 
> >>>> 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.
> >>
> > I prefer f/w to fix the DT before passing it to the kernel,
> > either by adding the "secure-status" property (set explicitly to
> > "disabled")
> > or by removing the job ring node(s) that are reserved.

According to the DT bindings doc mentioned by Michael below, it would not
be needed to remove the node.

Setting status = "disabled"; secure-status = "okay" should be enough to
reserve JR node in S World permanently. It would also serve the purpose
to indicate that the HW do provide the correct total amount of JRs, and
just some of then are not available to be used in NS World.

> > OP-TEE already uses the first option. We should probably pick this up.

Agree. I would drop the register access from this patch and follow-up with
DT node approach.

> 
> Ah, nice:
> Documentation/devicetree/bindings/arm/secure.txt

Good point, thanks for the doc guidance! This does provide a clear layout
on how the DT node should be crafted!

> 
> If I understand this correctly, if optee reserves a JR it will set the
> secure-status to okay and status to disabled. (There is still a missing
> link, how u-boot will then be passed this modified device tree, I might
> miss something here.)

I need to look at how OP-TEE does things here, but if they just set
secure-status = "okay" - then the JR should be visible in both worlds.

> 
> But what about the HAB, if I understand Andrey correct, then JR0 will
> already be marked as "S world only" (or at least no EL3 program will
> release it again).

It's a good point, which is still unclear: can JR0 be reclaimed back
after HAB is finished? Or should it stay in S-only world?

> To me it looks like then either JR0 should be
> (1) hardcoded to secure-status = "okay", status = "disabled", or (2)
> u-boot SPL (or TF-A) should return it to NS world (and optee might
> take it over again).

If the answer to HAB question is: it should stay in S World, then
I'd suggest to go with (1) as it presents the opportunity to define the
initial state of JR0 in deterministic state, without loosing the
information that HW does indeed have it implemented (node is present,
but permanently disabled). Later reclamation with this combination is
also possible.

What I would propose in addition here as well, is to define the
secure-status = "disabled" for all JR nodes on all derivatives, to have
the status set consistently. If later reclamation for any arbitrary JR
from NS to S world is needed - this property can be adjusted
accordingly by SW entity. Same thing should be done in U-Boot I suppose.

> 
> > The reason I am supporting relying on DT and consequently avoiding
> > registers
> > is that accessing page 0 in the caam register space from Non-secure
> > world
> > should be avoided when caam is managed by Secure world (e.g. OP-TEE)
> > or a Secure Enclave (e.g. SECO).

Understood, this is a valid point that I was missing. I'd re-work this
to use DT bindings and push it in V3.

I guess there would not be much left of this patch once I'd use DT approach,
so I'd re-spin the series to include DT bindings instead. JR driver clean-up
to remove static JR counter :| would go into the first one then.

> >
> > Unfortunately support for HW-enforced access control for caam register
> > space
> > is not that great / fine-grained, with the exception of more recent
> > parts
> > like i.MX8MP and i.MX8ULP.

Are there any particular distinct differences on those derivatives that
should be taken into account here with respect of JR reservation?
I might include those as well in this patch series if they are not that
significant, otherwise would try to address them separately.

> 
> -michael

Thanks to all of you for commenting here!

-- andrey