When dynamic shared memory support is enabled in the OP-TEE Trusted
OS, it doesn't mean that the driver supports it, which can confuse
users during debugging. Log a message when dynamic shared memory is
enabled in the driver, to let users know for sure.
Suggested-by: Jerome Forissier <jerome.forissier(a)linaro.org>
Signed-off-by: Victor Chong <victor.chong(a)linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier(a)linaro.org>
---
v2:
* Added patch description
[v1] lore.kernel.org/patchwork/patch/983949/
drivers/tee/optee/core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index e1aafe842d66..cca5c091e55a 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -631,6 +631,9 @@ static struct optee *optee_probe(struct device_node *np)
optee_enable_shm_cache(optee);
+ if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
+ pr_info("dynamic shared memory is enabled\n");
+
pr_info("initialized driver\n");
return optee;
err:
--
2.17.1
This bug occurs when:
- a new request arrives, one thread(let's call it A) is pending in
optee_supp_req() with req->busy is initial value false.
- tee-supplicant is killed, then optee_supp_release() is called, this
function calls list_del(&req->link), and set supp->ctx to NULL. And
it also wake up process A.
- process A continues, it firstly checks supp->ctx which is NULL,
then checks req->busy which is false, at last run list_del(&req->link).
This triggers double list_del() and results kernel panic.
For solve this problem, we rename req->busy to req->in_queue, and
associate it with state of whether req is linked to supp->reqs. So we
can just only check req->in_queue to make decision calling list_del()
or not.
Signed-off-by: Zhizhou Zhang <zhizhouzhang(a)asrmicro.com>
---
drivers/tee/optee/supp.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/supp.c b/drivers/tee/optee/supp.c
index df35fc01fd3e..43626e15703a 100644
--- a/drivers/tee/optee/supp.c
+++ b/drivers/tee/optee/supp.c
@@ -19,7 +19,7 @@
struct optee_supp_req {
struct list_head link;
- bool busy;
+ bool in_queue;
u32 func;
u32 ret;
size_t num_params;
@@ -54,7 +54,6 @@ void optee_supp_release(struct optee_supp *supp)
/* Abort all request retrieved by supplicant */
idr_for_each_entry(&supp->idr, req, id) {
- req->busy = false;
idr_remove(&supp->idr, id);
req->ret = TEEC_ERROR_COMMUNICATION;
complete(&req->c);
@@ -63,6 +62,7 @@ void optee_supp_release(struct optee_supp *supp)
/* Abort all queued requests */
list_for_each_entry_safe(req, req_tmp, &supp->reqs, link) {
list_del(&req->link);
+ req->in_queue = false;
req->ret = TEEC_ERROR_COMMUNICATION;
complete(&req->c);
}
@@ -103,6 +103,7 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
/* Insert the request in the request list */
mutex_lock(&supp->mutex);
list_add_tail(&req->link, &supp->reqs);
+ req->in_queue = true;
mutex_unlock(&supp->mutex);
/* Tell an eventual waiter there's a new request */
@@ -130,9 +131,10 @@ u32 optee_supp_thrd_req(struct tee_context *ctx, u32 func, size_t num_params,
* will serve all requests in a timely manner and
* interrupting then wouldn't make sense.
*/
- interruptable = !req->busy;
- if (!req->busy)
+ if (req->in_queue) {
list_del(&req->link);
+ req->in_queue = false;
+ }
}
mutex_unlock(&supp->mutex);
@@ -176,7 +178,7 @@ static struct optee_supp_req *supp_pop_entry(struct optee_supp *supp,
return ERR_PTR(-ENOMEM);
list_del(&req->link);
- req->busy = true;
+ req->in_queue = false;
return req;
}
@@ -318,7 +320,6 @@ static struct optee_supp_req *supp_pop_req(struct optee_supp *supp,
if ((num_params - nm) != req->num_params)
return ERR_PTR(-EINVAL);
- req->busy = false;
idr_remove(&supp->idr, id);
supp->req_id = -1;
*num_meta = nm;
--
2.17.1
This bug occurs when:
- a new request arrives, one thread(let's call it A) is pending in
optee_supp_req() with req->busy is initial value false.
- tee-supplicant is killed, then optee_supp_release() is called, this
function calls list_del(&req->link), and set supp->ctx to NULL. And
it also wake up process A.
- process A continues, it firstly checks supp->ctx which is NULL,
then checks req->busy which is false, at last run list_del(&req->link).
This triggers double list_del() and results kernel panic.
So let's set req->busy to true if optee_supp_release() has already
called list_del(&req->link).
Signed-off-by: Zhizhou Zhang <zhizhouzhang(a)asrmicro.com>
---
drivers/tee/optee/supp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tee/optee/supp.c b/drivers/tee/optee/supp.c
index df35fc01fd3e..c8ac6636520a 100644
--- a/drivers/tee/optee/supp.c
+++ b/drivers/tee/optee/supp.c
@@ -62,6 +62,7 @@ void optee_supp_release(struct optee_supp *supp)
/* Abort all queued requests */
list_for_each_entry_safe(req, req_tmp, &supp->reqs, link) {
+ req->busy = true;
list_del(&req->link);
req->ret = TEEC_ERROR_COMMUNICATION;
complete(&req->c);
--
2.17.1
+ OP-TEE ML.
On Fri, 2 Nov 2018 at 06:11, Chris Co <Christopher.Co(a)microsoft.com> wrote:
>
> Hi Sumit,
>
> Our full OpteeClientPkg has:
> - Our OpteeClientAPI implementation. I was monitoring the merge progress on OpteeLib and will look into moving over now that it is available.
> - The fTPM and AuthVar TA binaries. In our current design, the TA binaries are loaded at runtime. We could host the binaries themselves elsewhere on the filesystem, but we do not want these binaries as early/pseudo TAs. Is there a plan for OpteeLib to support loading full TAs?
Early TAs [1] are basically full TAs only, running in Secure EL0 mode.
So instead of loading TA from normal world file-system, they are
linked into a special data section in the OP-TEE core blob.
Also I don't think loading TAs dynamically especially during boot
makes much sense due to following reasons:
1. Increased boot time.
2. Fixed TAs like in your case which could be linked as early TAs as well.
And you mentioned filesystem, are you referring to root filesystem?
> - We have two client drivers: a firmware TPM TA driver and an authenticated variable TA driver. These talk through the tee-supplicant to their respective TAs.
>
Here from tee-supplicant apart from loading TAs, what other services
are you expecting? If you are looking for secure storage via RPMB,
that could be an enhancement to OpteeLib adding corresponding RPC
handling here [2].
[1] https://github.com/OP-TEE/optee_os/blob/master/documentation/optee_design.m…
[2] https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/OpteeLib/Optee…
Regards,
Sumit
> Chris
>
> > -----Original Message-----
> > From: Sumit Garg <sumit.garg(a)linaro.org>
> > Sent: Thursday, November 1, 2018 3:55 AM
> > To: Chris Co <Christopher.Co(a)microsoft.com>; Leif Lindholm
> > <leif.lindholm(a)linaro.org>
> > Cc: edk2-devel(a)lists.01.org; Ard Biesheuvel <ard.biesheuvel(a)linaro.org>;
> > Michael D Kinney <michael.d.kinney(a)intel.com>
> > Subject: Re: [PATCH edk2-platforms 01/27] Platform/Microsoft: Add
> > OpteeClientPkg dec
> >
> > Hi Christopher,
> >
> > Optee Client library has recently been merged to edk2 source code. It tries to
> > provide a generic interface [1] to OP-TEE based trusted applications
> > (pseudo/early).
> >
> > AFAIK, you don't need any platform specific hook in client interface to work
> > with upstream OP-TEE. So instead you should use Optee library.
> >
> > [1]
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> > om%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FArmPkg%2FInclude%2FLibrary
> > %2FOpteeLib.h&data=02%7C01%7CChristopher.Co%40microsoft.com%7C
> > c19b84ef7f8f4213424108d63fe88f66%7C72f988bf86f141af91ab2d7cd011db47
> > %7C1%7C0%7C636766665404786500&sdata=m24akbKtoyCERVN77meoSU
> > H6E%2Bpf8W2P5MF7nvU5y7I%3D&reserved=0
> >
> > Regards,
> > Sumit
> >
> > On Thu, 1 Nov 2018 at 02:13, Leif Lindholm <leif.lindholm(a)linaro.org> wrote:
> > >
> > > +Sumit (just to loop you two together). Is there anything Microsoft
> > > platform specific about what will go in here?
> > >
> > > /
> > > Leif
> > >
> > > On Fri, Sep 21, 2018 at 08:25:53AM +0000, Chris Co wrote:
> > > > On Windows IoT Core devices with ARM TrustZone capabilities,
> > > > EDK2 runs in normal world and we use OP-TEE to execute secure world
> > > > operations. The overall package will contain client-side support to
> > > > invoke EDK2 services implemented as OP-TEE trusted applications that
> > > > run in secure world.
> > > >
> > > > This commit adds the initial dec file to add some PCD settings
> > > > needed by other packages.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Christopher Co <christopher.co(a)microsoft.com>
> > > > Cc: Ard Biesheuvel <ard.biesheuvel(a)linaro.org>
> > > > Cc: Leif Lindholm <leif.lindholm(a)linaro.org>
> > > > Cc: Michael D Kinney <michael.d.kinney(a)intel.com>
> > > > ---
> > > > Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec | 49
> > > > ++++++++++++++++++++
> > > > 1 file changed, 49 insertions(+)
> > > >
> > > > diff --git a/Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec
> > > > b/Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec
> > > > new file mode 100644
> > > > index 000000000000..4752eab39ce3
> > > > --- /dev/null
> > > > +++ b/Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec
> > > > @@ -0,0 +1,49 @@
> > > > +## @file
> > > > +#
> > > > +# OP-TEE client package
> > > > +#
> > > > +# OP-TEE client package contains the client-side interface to invoke OP-
> > TEE TAs.
> > > > +# Certain EDKII services are implemented in Trusted Applications
> > > > +running in # the secure world OP-TEE OS.
> > > > +#
> > > > +# Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > > > +#
> > > > +# This program and the accompanying materials # are licensed and
> > > > +made available under the terms and conditions of the BSD License #
> > > > +which accompanies this distribution. The full text of the license
> > > > +may be found at #
> > > > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> > > > +nsource.org%2Flicenses%2Fbsd-
> > license.php&data=02%7C01%7CChristo
> > > >
> > +pher.Co%40microsoft.com%7Cc19b84ef7f8f4213424108d63fe88f66%7C72f988
> > > >
> > +bf86f141af91ab2d7cd011db47%7C1%7C0%7C636766665404786500&sda
> > ta=1
> > > > +MxFvlsMPhk19grEexBXo5VqRd0jZaCSRjxZCi87A2w%3D&reserved=0
> > > > +#
> > > > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > > > +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> > EITHER EXPRESS OR IMPLIED.
> > > > +#
> > > > +##
> > > > +
> > > > +[Defines]
> > > > + DEC_SPECIFICATION = 0x0001001A
> > > > + PACKAGE_NAME = OpteeClientPkg
> > > > + PACKAGE_GUID = 77416fcb-10ec-4693-bdc0-1bdd74ec9595
> > > > + PACKAGE_VERSION = 0.01
> > > > +
> > > > +[Includes]
> > > > +
> > > > +[LibraryClasses]
> > > > +
> > > > +[Guids]
> > > > + gOpteeClientPkgTokenSpaceGuid = { 0x04ad34ca, 0xdd25, 0x4156, {
> > 0x90, 0xf5, 0x16, 0xf9, 0x40, 0xd0, 0x49, 0xe3 }}
> > > > +
> > > > +[PcdsFixedAtBuild]
> > > > +
> > > >
> > +gOpteeClientPkgTokenSpaceGuid.PcdTpm2AcpiBufferBase|0|UINT64|0x0000
> > > > +0005
> > > > +
> > > >
> > +gOpteeClientPkgTokenSpaceGuid.PcdTpm2AcpiBufferSize|0|UINT32|0x0000
> > > > +0006
> > > > +
> > > > + ## The base address of the Trust Zone OpTEE OS private memory
> > > > + region # This memory is manager privately by the OpTEE OS.
> > > > +
> > > > +
> > gOpteeClientPkgTokenSpaceGuid.PcdTrustZonePrivateMemoryBase|0xDEAD
> > > > + 1|UINT64|0x00000001
> > > > +
> > > > + ## The size of the Trust Zone OpTEE OS private memory region
> > > > +
> > > > +
> > gOpteeClientPkgTokenSpaceGuid.PcdTrustZonePrivateMemorySize|55|UIN
> > > > + T64|0x00000002
> > > > +
> > > > + ## The base address of the Trust Zone OpTEE OS shared memory
> > > > + region
> > > > +
> > > > +
> > gOpteeClientPkgTokenSpaceGuid.PcdTrustZoneSharedMemoryBase|0xDEAD2
> > > > + |UINT64|0x00000003
> > > > +
> > > > + ## The size of the Trust Zone OpTEE OS shared memory region
> > > > +
> > > > +
> > gOpteeClientPkgTokenSpaceGuid.PcdTrustZoneSharedMemorySize|0xAA|UI
> > > > + NT64|0x00000004
> > > > --
> > > > 2.16.2.gvfs.1.33.gf5370f1
> > > >
Changes in v5:
Define RFC4122_UUID struct using network byte order instead of 16 byte
octects for passing Trusted Application UUID.
Changes in v4:
Replaced abbreviations with full name which are not defined in [1]. Also
used EFI_GUID for Trusted Application UUIDs.
[1] https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/conte…
Changes in v3:
Removed GlobalPlatform TEE return codes (IndustryStandard/GlobalPlatform.h)
that were rejected by EDK2 maintainers. Rather used custom ones for this
OP-TEE driver.
Changes in v2:
1. Separate patch for MdePkg/Include/IndustryStandard/GlobalPlatform.h.
2. Correct comments style for struct members.
3. Update commit message.
Sumit Garg (1):
ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE
ArmPkg/Library/OpteeLib/OpteeLib.inf | 2 +
ArmPkg/Include/Library/OpteeLib.h | 88 +++++
ArmPkg/Library/OpteeLib/OpteeSmc.h | 53 +++
ArmPkg/Library/OpteeLib/Optee.c | 392 ++++++++++++++++++++
4 files changed, 535 insertions(+)
create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h
--
2.7.4
