| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFA6WYMuAgL0D7oQYoK_zaJ+2hjtyeeVHOgA4tfkurzGuQi6=g@mail.gmail.com>
Date: Fri, 27 Aug 2021 10:57:00 +0530
From: Sumit Garg <sumit.garg@...aro.org>
To: Jens Wiklander <jens.wiklander@...aro.org>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
OP-TEE TrustedFirmware <op-tee@...ts.trustedfirmware.org>,
Sudeep Holla <sudeep.holla@....com>,
Marc Bonnici <marc.bonnici@....com>,
Jerome Forissier <jerome@...issier.org>,
Sughosh Ganu <sughosh.ganu@...aro.org>
Subject: Re: [PATCH v4 5/5] optee: add FF-A support
On Thu, 26 Aug 2021 at 20:22, Jens Wiklander <jens.wiklander@...aro.org> wrote:
>
> On Wed, Aug 25, 2021 at 05:12:45PM +0530, Sumit Garg wrote:
> > On Thu, 19 Aug 2021 at 16:37, Jens Wiklander <jens.wiklander@...aro.org> wrote:
> > >
> > > Adds support for using FF-A [1] as transport to the OP-TEE driver.
> > >
> > > Introduces struct optee_msg_param_fmem which carries all information
> > > needed when OP-TEE is calling FFA_MEM_RETRIEVE_REQ to get the shared
> > > memory reference mapped by the hypervisor in S-EL2. Register usage is
> > > also updated to include the information needed.
> > >
> > > The FF-A part of this driver is enabled if CONFIG_ARM_FFA_TRANSPORT is
> > > enabled.
> > >
> > > [1] https://developer.arm.com/documentation/den0077/latest
> > > Signed-off-by: Jens Wiklander <jens.wiklander@...aro.org>
> > > ---
> > > drivers/tee/optee/Makefile | 3 +-
> > > drivers/tee/optee/call.c | 13 +-
> > > drivers/tee/optee/core.c | 16 +-
> > > drivers/tee/optee/ffa_abi.c | 907 ++++++++++++++++++++++++++++++
> > > drivers/tee/optee/optee_ffa.h | 153 +++++
> > > drivers/tee/optee/optee_msg.h | 27 +-
> > > drivers/tee/optee/optee_private.h | 43 +-
> > > 7 files changed, 1148 insertions(+), 14 deletions(-)
> > > create mode 100644 drivers/tee/optee/ffa_abi.c
> > > create mode 100644 drivers/tee/optee/optee_ffa.h
> > >
> [snip]
> > > --- /dev/null
> > > +++ b/drivers/tee/optee/ffa_abi.c
> > > @@ -0,0 +1,907 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2021, Linaro Limited
> > > + */
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/arm_ffa.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/string.h>
> > > +#include <linux/tee_drv.h>
> > > +#include <linux/types.h>
> > > +#include "optee_private.h"
> > > +#include "optee_ffa.h"
> > > +#include "optee_rpc_cmd.h"
> > > +
> > > +/*
> > > + * This file implement the FF-A ABI used when communicating with secure world
> > > + * OP-TEE OS via FF-A.
> > > + * This file is divided into the follow sections:
> >
> > s/follow/following/
>
> Thanks, I'll fix.
>
> [snip]
> > > +static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > + const struct ffa_dev_ops *ops,
> > > + unsigned int *rpc_arg_count)
> > > +{
> > > + struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > > + int rc;
> > > +
> > > + rc = ops->sync_send_receive(ffa_dev, &data);
> > > + if (rc) {
> > > + pr_err("Unexpected error %d", rc);
> > > + return false;
> > > + }
> > > + if (data.data0) {
> > > + pr_err("Unexpected exchange error %lu", data.data0);
> > > + return false;
> > > + }
> > > +
> > > + *rpc_arg_count = (u8)data.data1;
> >
> > Why is this special capability required in case of FF-A? Is it true
> > that RPC arguments count will be fixed for all RPC commands?
>
> It's to allow this driver to preallocate the argument struct used when
> doing RPC. That way we can avoid the chicken and egg problem of allocating
> an RPC argumet struct just before doing the real RPC.
>
> This is the maximum number of arguments needed by secure world. In case
> a larger value ever is needed, secure world will be able to supply the
> needed value.
>
> I plan to update the SMC based ABI with this also, but not in the patch
> set.
>
Okay, I see the requirement with FF-A ABI that we need to pass memory
reference to "struct thread_rpc_arg" while in case of SMC ABI we
directly pass the RPC commands in registers which allows it to work
without pre-allocation.
So I am fine with this approach as well given that the pre-allocated
memory for RPC arguments may be left unused in some cases but that's
fine when compared with the overhead of extra RPC calls with SMC ABI.
> >
> > > +
> > > + return true;
> > > +}
>
> [snip]
> > > +static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > +{
> > > + const struct ffa_dev_ops *ffa_ops;
> > > + unsigned int rpc_arg_count;
> > > + struct tee_device *teedev;
> > > + struct optee *optee;
> > > + int rc;
> > > +
> > > + ffa_ops = ffa_dev_ops_get(ffa_dev);
> > > + if (!ffa_ops) {
> > > + pr_warn("failed \"method\" init: ffa\n");
> > > + return -ENOENT;
> > > + }
> > > +
> > > + if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > > + return -EINVAL;
> > > +
> > > + if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_arg_count))
> > > + return -EINVAL;
> > => +
> > > + optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > > + if (!optee) {
> > > + rc = -ENOMEM;
> > > + goto err;
> > > + }
> > > + optee->pool = optee_ffa_config_dyn_shm();
> > > + if (IS_ERR(optee->pool)) {
> > > + rc = PTR_ERR(optee->pool);
> > > + optee->pool = NULL;
> > > + goto err;
> > > + }
> >
> > IIUC, with FF-A we will only be supporting dynamic shared memory. So
> > CFG_CORE_DYN_SHM=y should be enforced in OP-TEE OS when
> > CFG_CORE_FFA=y, but I don't see that currently. Am I missing
> > something?
>
> You mean in optee_os.git? With FF-A dynamic shared memory is always
> handled, so that flag in irrelevant in that case. However, feel free to
> start a discussion on that topic at github.
Ah, I see. You are correct that FF-A enables dynamic shared memory by default.
FWIW, feel free to add:
Acked-by: Sumit Garg <sumit.garg@...aro.org>
-Sumit
>
> Thanks,
> Jens
Powered by blists - more mailing lists