On Thu, Mar 17, 2022 at 1:17 PM Sumit Garg sumit.garg@linaro.org wrote:
On Wed, 16 Mar 2022 at 14:57, Jens Wiklander jens.wiklander@linaro.org wrote:
On Mon, Mar 14, 2022 at 02:33:26PM +0530, Sumit Garg wrote:
On Wed, 2 Mar 2022 at 01:18, Jens Wiklander jens.wiklander@linaro.org wrote:
Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that OP-TEE with FF-A can support an argument struct at a non-zero offset into a passed shared memory object.
It isn't clear to me why FF-A ABI requires this capability. shm->offset should be honoured by default, if it isn't then it's a bug.
Yes, there was a bug in secure world when using a non-zero offset. So far the driver has always used a zero offset that's why it hasn't been caught earlier.
It's not a serious bug, but it might be quite hard to track down. This is of course fixed now, but there is a window where it can be triggered.
I am not able to find the fix in this [1] OP-TEE OS commit. Could you help me to point it out?
[1] https://github.com/OP-TEE/optee_os/commit/89d991352352b80ae90f82228a014bb8ca...
I believe it's this one: https://github.com/OP-TEE/optee_os/commit/7267624eef019476f20aee7dba11ff949d...
So in order to spare FF-A developers this problem I'm making a non-zero offset an extension guarded by a capability bit. Even though this is an ABI change it's in practice not since it has been unused and not working as expected.
The next commit will start using non-zero offsets if supported, so this will change to become a problem (if left unchecked) in the window mentioned above.
AFAIK, FF-A is currently still in its early stages so it shouldn't be that hard to fix bugs in the ABI.
Starting from the kernel release (v5.16) where FF-A support in this driver was merged the ABI is supposed to be stable.
From Linux kernel perspective, there are dedicated stable branches for that purpose in case we want to push a fix for OP-TEE FF-A kernel driver.
The problem isn't in the kernel driver. What I'm doing here is to formalize the ABI that the kernel defacto was using. Since we obviously didn't test the driver with a non-zero offset before, we didn't notice that the offset wasn't used in the right way by the secure world.
Cheers, Jens
-Sumit
Thanks, Jens
-Sumit
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/optee/ffa_abi.c | 17 +++++++++++++++-- drivers/tee/optee/optee_ffa.h | 12 +++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c index 7686f7020616..cc863aaefcd9 100644 --- a/drivers/tee/optee/ffa_abi.c +++ b/drivers/tee/optee/ffa_abi.c @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx, .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG, .data1 = (u32)shm->sec_world_id, .data2 = (u32)(shm->sec_world_id >> 32),
.data3 = shm->offset,
.data3 = 0, }; struct optee_msg_arg *arg; unsigned int rpc_arg_offs; struct optee_msg_arg *rpc_arg;/** The shared memory object has to start on a page when passed as* an argument struct. This is also what the shm pool allocator* returns, but check this before calling secure world to catch* eventual errors early in case something changes.*/if (shm->offset)return -EINVAL;arg = tee_shm_get_va(shm, 0); if (IS_ERR(arg)) return PTR_ERR(arg);@@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev, const struct ffa_dev_ops *ops,
u32 *sec_caps, unsigned int *rpc_param_count){ struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES }; @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev, }
*rpc_param_count = (u8)data.data1;
*sec_caps = data.data2; return true;} @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) struct tee_device *teedev; struct tee_context *ctx; struct optee *optee;
u32 sec_caps; int rc; ffa_ops = ffa_dev_ops_get(ffa_dev);@@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops)) return -EINVAL;
if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,&rpc_param_count)) return -EINVAL; optee = kzalloc(sizeof(*optee), GFP_KERNEL);diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h index ee3a03fc392c..97266243deaa 100644 --- a/drivers/tee/optee/optee_ffa.h +++ b/drivers/tee/optee/optee_ffa.h @@ -81,8 +81,16 @@
as the second MSG arg struct forOPTEE_FFA_YIELDING_CALL_WITH_ARG.Bit[31:8]: Reserved (MBZ)
- w5-w7: Note used (MBZ)
- w5: Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
unused bits MBZ.
- w6-w7: Not used (MBZ)
- */
+/*
- Secure world supports giving an offset into the argument shared memory
*/
- object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
+#define OPTEE_FFA_SEC_CAP_ARG_OFFSET BIT(0)
#define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
/* @@ -112,6 +120,8 @@
OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_argfor RPC, this struct has reserved space for the number of RPCparameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
OPTEE_FFA_EXCHANGE_CAPABILITIES.- w7: Not used (MBZ)
- Resume from RPC. Register usage:
- w3: Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
-- 2.31.1