Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] core: implement OCALLs #3673

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HernanGatta
Copy link

@HernanGatta HernanGatta commented Mar 18, 2020

Overview

This PR implements Out Calls, or OCALLs for short. It allows a TA to invoke functions on its CA, effectively TEEC_InvokeCommand in reverse.

This is a re-implementation of #3292, but all processing is done in-thread. That is, the normal world thread that invokes a command on a TA, which results in an OCALL, is the one that handles the OCALL, as opposed to using an out-of-thread call queue.

Related PRs: Linux, OP-TEE Client, OP-TEE Examples.

Note: PRs into the driver, the client library, the examples, and the test suite coming shortly. I'll update the GitHub issue we were discussing this on before once the PR's are up (assuming I can re-open it.)

The life of an OCALL

When opening a session, the CA uses a new API: TEEC_OpenSessionEx. This function optionally takes a list of typed settings. One such setting is the OCALL setting, which allows the developer to set the OCALL handler function, and an arbitrary context pointer. The function goes on to ascertain that OP-TEE reports OCALL support. If so, the function uses TEEC_OpenSession under the covers to open a session. Lastly, it sets the OCALL setting in the session object.

From this point forward, by virtue of having configured OCALLs, the CA's calls into TEEC_InvokeCommand are passed via a new IOCTL in the TEE driver called Enclave Call, or ECALL for short. The handler for this IOCTL in the OP-TEE driver initiates a regular function invocation into OP-TEE. If the TA returns in the usual way, that is, it does not request an OCALL, the IOCTL behaves just the same as the Invoke one.

On the other hand, the TA can call TEE_InvokeCACommand, whose signature is similar to TEEC_InvokeTACommand. This function, implemented in libutee, calls into the OCALL PTA.

The OCALL PTA examines the OCALL parameters, and determines if any of them are memory references. If so, it computes their aggregate size and sends an RPC to normal world to request that the CA allocate shared memory of the given size. The driver stack picks up on the request, creates an OCALL context to hold the current call and RPC context, gives it an ID, and the ECALL IOCTL completes.

As part of the IOCTL completion, its values and parameters are now completely changed. A func element indicates the function that the TEE Client API must execute, in this case "Allocate shared memory", while the parameters carry the size of the requested buffer. Additionally, the ID of the OCALL context is included for resumption later.

The TEE Client API allocates shared memory the same way the TEE Supplicant does (i.e., if registered memory is available, it is not actually registered with OP-TEE). Once this is done, the result of the operation is wrapped in the IOCTL parameters and a new ECALL IOCTL is issued.

The ECALL IOCTL's logic picks up on the OCALL ID and looks up the OCALL context. The func value remains unchanged, so the IOCTL knows this is a reply to the shared memory allocation. After some simple parameter processing, the result is returned to OP-TEE via an RPC resume.

At this stage, the OCALL PTA resumes execution. Its next step is to set up RPC parameters for the OCALL RPC request. The maximum number of RPC parameters is increased to six. As such, the first RPC parameter includes the Command ID for the CA to execute, the second includes the UUID of the TA whence the OCALL request originated, and the remaining four RPC parameters are filled with those of the OCALL, after processing. Processing involves copying values, and copying memory references from TA-mapped memory into the shared memory allocated previously, if necessary. A new RPC is issued.

The RPC is picked up by normal word. The OCALL context is retrieved again, call context is saved, and the ECALL IOCTL's values are updated. The func parameter now indicates that the TEE Client API must pass the OCALL request to the CA proper, and the cmd_id value stores the Command ID that the CA knows how to interpret (i.e., the OCALL Id). The parameters are those of the OCALL as it originated in the TA, again after proper processing. The ECALL IOCTL completes.

The TEE Client API invokes the OCALL handler set previously via session settings and the CA finally gains control. The handler gets its arbitrary context pointer, the UUID of the requesting TA, and the OCALL parameters. It can modify INOUT and OUTPUT parameters freely. Once the OCALL handler returns, the TEE Client API processes parameters as needed and issues a new ECALL IOCTL, including the return value of the OCALL and its origin code.

The ECALL IOCTL recognizes this invocation as a resumption by its OCALL ID parameter, and knows that it's a response to an OCALL request by examining the func parameter. It performs the appropriate parameter processing and returns to OP-TEE with an RPC resume. The first parameter of the RPC is modified to pass the OCALL return value and origin code.

The OCALL PTA gains control once more. It processes the returned parameters, copying values and buffers as necessary into TA-mapped memory. If shared memory was allocated previously, another round of ECALL IOCTL completion followed by a reply is executed to have the CA free the memory.

All is now done and control is returned to the TA, including the return value and origin code of the OCALL.

Signed-off-by: Hernan Gatta [email protected]

lib/libutee/include/tee_api_defines.h Outdated Show resolved Hide resolved
{ 0x05ece642, 0x903f, 0x47bf, \
{ 0x94, 0xeb, 0x07, 0xd6, 0x14, 0x61, 0x73, 0xa1 } }

#define PTA_OCALL_SEND 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*
 * Invoke a RPC Output Call to CA 
 *
 * memref[0..3] can be any parameter type and are provide CA OCall handler
 */

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -547,3 +547,6 @@ CFG_SHOW_CONF_ON_BOOT ?= n
# to a TA, so a TPM Service could use it to extend any measurement
# taken before the service was up and running.
CFG_CORE_TPM_EVENT_LOG ?= n

# Enables support for OCALLs
CFG_OCALL ?= y
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CFG_CA_OCALL?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think _CA_ would add anything useful. The name of the feature is OCALL.
The comment describing the config could mention that this is an RPC into the Client Application.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more info.

@@ -148,6 +150,20 @@
*/
#define OPTEE_RPC_CMD_BENCH_REG 20

/*
* Send a command to the Client Application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Send an OCall command to the Client Application."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

core/pta/ocall.c Outdated
return TEE_SUCCESS;
}

#define MEMREF_CHECK_AND_SET_RPC(x) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer a function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, will simplify; this is ugly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@HernanGatta
Copy link
Author

HernanGatta commented Mar 22, 2020

A note on the style fixes commit: I assumed, apparently incorrectly, that commits would be squashed before merging, but that's not the strategy you've all been using. That's my mistake in working the way I've been working on other projects. So, I'll re-write the commit history into something that can be linearly appended to master, and the same for the other related PRs.

As-is, fixing checkpatch errors on each commit is going to be a lot of trouble. I've tried rebasing interactively atop master, and fixing errors on each commit, but that just causes merge conflicts as the rebase progresses, and I'd be fixing errors on code that was ultimately deleted or re-written anyway, so it seems like a waste of time.

Let me know if I got this wrong.

@jforissier
Copy link
Contributor

@HernanGatta you're right, in the end we expect a set of properly formatted patches, each one addressing a specific need or adding a specific feature.

Coding style fixes can indeed be pain to do during rebase... my suggestion would be to first re-organize your current set of patches using git rebase -i. You would squash the things that make sense together and write proper descriptions, but you would not address any coding style issue yet. Then you would rebase again and use the "edit" option to re-format where needed. By doing so you will at least avoid issues with code that is added then removed.

core/pta/ocall.c Outdated
case TEE_PARAM_TYPE_NONE:
rpc_params[n].attr = THREAD_PARAM_ATTR_NONE;
break;
case TEE_PARAM_TYPE_VALUE_INPUT:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value types could be written more compactly as (assuming that rpc_params is zero initialized):

case TEE_PARAM_TYPE_VALUE_INPUT:
case TEE_PARAM_TYPE_VALUE_INOUT:
        rpc_params[n].u.value.a = ca_param->value.a;
        rpc_params[n].u.value.b = ca_param->value.b;
        /*FALLTHROUGH*/
case TEE_PARAM_TYPE_VALUE_OUTPUT:
        rpc_params[n].attr = THREAD_PARAM_ATTR_VALUE_IN + ca_pt -
                             TEE_PARAM_TYPE_VALUE_INPUT;
        break;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I like it; I wanted to get rid of the duplication but couldn't really due to the macro. I had seen code like what you suggested elsewhere in OP-TEE, but I figured that since the macro to set thread parameters was there, one is meant to use them.

Thank you for the suggestion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as suggested.

core/pta/ocall.c Outdated
ca_param->value.b,
0);
break;
case TEE_PARAM_TYPE_MEMREF_INPUT:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The memref code here could also be rewritten similarly to what's suggested for value params above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as suggested.

core/pta/ocall.c Outdated
ca_cmd_id = params[0].value.a;

/* Process OCALL parameters, if any */
if (pt2 == TEE_PARAM_TYPE_MEMREF_INOUT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about skipping this optimization and instead always require the TA to always supply this?

Copy link
Author

@HernanGatta HernanGatta Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section 4.9.4 of the TEE Internal Core API v1.2.1 spec. states about calling TEE_InvokeTACommand:

Unless all parameter types are set to TEE_PARAM_TYPE_NONE, params SHALL NOT be NULL [...]

I'd like TEE_InvokeCACommand to behave as closely to TEE_InvokeTACommand as possible. So, params can be NULL if all parameter types are NONE. When this is the case, to remove the check you highlighted, TEE_InvokeCACommand would have to pass a dummy array to the PTA.

I'd argue it's best to have this check, than the alternative. Thoughts?

Edit: Perhaps dealing with the issue of an AArch32 TA running on an AArch64 kernel may cancel out this problem.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEE_InvokeCACommand() passing a dummy array sounds good.

core/pta/ocall.c Outdated
/* Process OCALL parameters, if any */
if (pt2 == TEE_PARAM_TYPE_MEMREF_INOUT) {
ca_param_types = params[0].value.b;
ca_params = (TEE_Param *)params[1].memref.buffer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TA may execute in AArch32 while we're at AArch64 here so TEE_Param will have a different layout.

Copy link
Author

@HernanGatta HernanGatta Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the way you've worked around this problem elsewhere is by turning TEE_Param into utee_param, then back again, for transport across the UM/KM boundary. I've taken the same approach.

Do you think it's fine that way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

core/pta/ocall.c Outdated
return TEE_ERROR_BAD_PARAMETERS;

/* Compute memory the CA must allocate for the OCALL params */
res = compute_required_mobj_size(ca_param_types, ca_params,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents zero copy.
How about letting the TA allocate the buffers first and pass the pointers to those in the ca_params above?
That would simplify this function a bit too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so TA would need to allocate non-secure memory. There isn't yet any API for that. Consider a specific command of this OCALL PTA?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it would be an extension which would go together with the OCALL extension.
By the way, I think all this should go into the system PTA.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, it's not trivial in that what is being suggested is registered memref-type functionality, and I don't think we have enough fields in TEE_Param, and associated structures, to make this work, at least not without changing them.

I'm thinking about it...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can take this after this PR. It's only going to affect the ABI between TA and OP-TEE, so we can let that change with regards to OCALL for a while.

@@ -547,3 +547,6 @@ CFG_SHOW_CONF_ON_BOOT ?= n
# to a TA, so a TPM Service could use it to extend any measurement
# taken before the service was up and running.
CFG_CORE_TPM_EVENT_LOG ?= n

# Enables support for OCALLs
CFG_OCALL ?= y
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think _CA_ would add anything useful. The name of the feature is OCALL.
The comment describing the config could mention that this is an RPC into the Client Application.

lib/libutee/tee_api.c Outdated Show resolved Hide resolved
lib/libutee/tee_api.c Outdated Show resolved Hide resolved
@@ -255,6 +260,11 @@ static TEE_Result entry_close_session(unsigned long session_id)

TA_CloseSessionEntryPoint(session->session_ctx);

#if defined(CFG_OCALL)
if (tee_api_ocall_session)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid the #if (here and above) with the following? (untested):

	if (IS_ENABLED(CFG_OCALL) && tee_api_ocall_session)
		...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do this, but then we'd have to always declare tee_api_ocall_session, regardless of the value of CFG_OCALL. That is, if OCALL support is disabled, that variable would still exist. I would imagine that the linker would realize it's untouched and discard it.

Preference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would imagine that the linker would realize it's untouched and discard it.

Indeed, because we're using -fdata-sections and --gc-sections.
I think we should try to use IS_ENABLED() as much as possible, because it avoids #ifdef which is good for legibility and also build coverage in CI.

Copy link
Author

@HernanGatta HernanGatta Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I've also removed the #ifdef from tee_api.c.

Would you like to see IS_ENABLED for capability reporting, too?

We have there another case of a different existing style, in this case not using the macro.

core/pta/ocall.c Outdated Show resolved Hide resolved
core/pta/ocall.c Outdated
return TEE_ERROR_BAD_PARAMETERS;

/* Compute memory the CA must allocate for the OCALL params */
res = compute_required_mobj_size(ca_param_types, ca_params,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so TA would need to allocate non-secure memory. There isn't yet any API for that. Consider a specific command of this OCALL PTA?

lib/libutee/include/tee_api_defines.h Outdated Show resolved Hide resolved
core/pta/ocall.c Outdated
TEE_Param params[TEE_NUM_PARAMS],
size_t *required_size)
{
TEE_Param *param;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generic comment: always initialize local variables where defined. See coding-standards, 3rd bullet.

Copy link
Author

@HernanGatta HernanGatta Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. This requirement cuts both ways though: always initializing variables precludes the compiler from warning the developer when the variable is used in a location where its value should have been updated prior to the usage. I've run into a few of those in fact while writing this patch set.

From my days of working on Windows, we'd leave uninitialized variables which are expected to be filled later on. That way, if our control flow is busted and a variable is used without having been set to the expected value, the compiler would bark at us. And we compile with warnings as errors, so we'd fail the build altogether.

I'll follow the coding standards, but with reservations :) .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale is that we want all variables to have a well defined value, i.e., to have the developer think about it and come up with a good value instead of having what often is a random value otherwise (static's etc are an exception). The reason why we've introduced this is because we've been bitten by a couple of security issues in the past due to uninitialized variables. As anything else, there are pros and cons.

@HernanGatta HernanGatta force-pushed the optee-generic-rpc-support branch 3 times, most recently from 4008dfc to 8eaec00 Compare March 26, 2020 05:04
@HernanGatta
Copy link
Author

HernanGatta commented Mar 26, 2020

@jforissier

in the end we expect a set of properly formatted patches

All fixed up, except for this:

CHECK: Prefer using the BIT macro
#25: FILE: core/arch/arm/include/sm/optee_smc.h:280:
+#define OPTEE_SMC_SEC_CAP_OCALL			(1 << 4)

I could easily change that to BIT(4), but that wouldn't follow the style of the rest of the file.

Would you like me to fix up the rest of the file with BIT(x), or do we ignore this complaint?

@jforissier
Copy link
Contributor

@jforissier

in the end we expect a set of properly formatted patches

All fixed up, except for this:

CHECK: Prefer using the BIT macro
#25: FILE: core/arch/arm/include/sm/optee_smc.h:280:
+#define OPTEE_SMC_SEC_CAP_OCALL			(1 << 4)

I could easily change that to BIT(4), but that wouldn't follow the style of the rest of the file.

Would you like me to fix up the rest of the file with BIT(x), or do we ignore this complaint?

I'd rather fix that too since I see no reason not to ;-) Please introduce a prior commit changing the existing defines.

@jbech-linaro
Copy link
Contributor

@jforissier

in the end we expect a set of properly formatted patches

All fixed up, except for this:

CHECK: Prefer using the BIT macro
#25: FILE: core/arch/arm/include/sm/optee_smc.h:280:
+#define OPTEE_SMC_SEC_CAP_OCALL			(1 << 4)

I could easily change that to BIT(4), but that wouldn't follow the style of the rest of the file.
Would you like me to fix up the rest of the file with BIT(x), or do we ignore this complaint?

I'd rather fix that too since I see no reason not to ;-) Please introduce a prior commit changing the existing defines.

Also, please change it to use "5" instead of "4", since we already have other pull requests (NULL parameter support patches) in the pipe where "4" is used.

@HernanGatta HernanGatta force-pushed the optee-generic-rpc-support branch 3 times, most recently from 7032624 to a269d46 Compare March 26, 2020 21:43
@HernanGatta
Copy link
Author

The rationale is [..]

Thank for you the explanation. Seeing the security rationale in the coding standards does certainly make sense, one wouldn't want to leak stack data inadvertently.

I'd rather fix that too since I see no reason not to ;-) Please introduce a prior commit changing the existing defines.

Done.

Also, please change it to use "5" instead of "4", since we already have other pull requests (NULL parameter support patches) in the pipe where "4" is used.

Good call, done.

@HernanGatta HernanGatta force-pushed the optee-generic-rpc-support branch 2 times, most recently from ec11de8 to b3d485f Compare March 26, 2020 22:20
@HernanGatta
Copy link
Author

HernanGatta commented Mar 26, 2020

All green, cleaning up the Linux PR next.

Outstanding issues

  1. Do we move all this to the system PTA?
  2. Are we all OK with the current nomenclature?
  3. Zero-copy now or later?
  4. Any other concerns?

My answers

  1. Sounds fine by me.
    1.1. While I'm not arguing for the following, I wonder: would anyone suggest turning this functionality into a system call seeing as TEE_InvokeTACommand is backed by one?
  2. I am.
  3. We could do it later for no other reason than to get the current changes moving.
    3.1 Can anyone think of any risks regarding, say, backwards compatibility, or otherwise, if we wait?
  4. I'd like to follow up as suggested about the known issue in the PR description.

@jenswi-linaro
Copy link
Contributor

1.1. While I'm not arguing for the following, I wonder: would anyone suggest turning this functionality into a system call seeing as TEE_InvokeTACommand is backed by one?

We don't add new syscalls unless there's no other practical way.

  1. Zero-copy now or later?
  2. We could do it later for no other reason than to get the current changes moving.
    3.1 Can anyone think of any risks regarding, say, backwards compatibility, or otherwise, if we wait?

There is the backwards compatibility issue with the ABI change, the API will remain the same the TA just needs to be recompiled or at least relinked.

  1. I'd like to follow up as suggested about the known issue in the PR description.

Please contact me privately, [email protected] and I'll take a look.

@HernanGatta HernanGatta force-pushed the optee-generic-rpc-support branch 5 times, most recently from 4b5a6b6 to c44578b Compare April 11, 2020 07:44
@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@jforissier
Copy link
Contributor

@HernanGatta do you think you can spend some more time addressing the last comments on this series? AFAICT there is not much left to do before this can be merged (and same for the other PRs: libteec, xtest and the linux driver). It would be really nice to have this in the next release (3.10.0) which is planned for August 21st.

@HernanGatta HernanGatta force-pushed the optee-generic-rpc-support branch 2 times, most recently from bc25eb0 to 8f56808 Compare October 19, 2020 05:54
@HernanGatta
Copy link
Author

HernanGatta commented Oct 19, 2020

@HernanGatta do you think you can spend some more time addressing the last comments on this series? AFAICT there is not much left to do before this can be merged (and same for the other PRs: libteec, xtest and the linux driver). It would be really nice to have this in the next release (3.10.0) which is planned for August 21st.

Unfortunately, the timing on my end didn't line up. Hopefully we can get this in by 3.12.0 in January, though.

I've addressed your comments. An additional change is that I've set CFG_OCALL=n if CFG_CORE_SEL1_SPMC=y. I'm not familiar with FF-A and the corresponding build did not pass seeing as the RPC functions for memory allocation and free are reimplemented for that case in a different way.

I presume there should be a way to still support OCALLs on FF-A, though. Is it possible to emulate FF-A with QEMU?

Also, I can remove the Reviewed-by tag from core: OCALL support if you'd like another pass at this considering new changes in OP-TEE since your last look.

Thanks!

@jenswi-linaro
Copy link
Contributor

Disabling OCALL now if FF-A is in use is OK in my opinion. FF-A is still in an experimental state. Adding OCALL support with FF-A later on when things has settled should not be hard.

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one small concern below.

lib/libutee/tee_api.c Outdated Show resolved Hide resolved
lib/libutee/arch/arm/user_ta_entry.c Outdated Show resolved Hide resolved
lib/libutee/tee_api.c Show resolved Hide resolved
@HernanGatta HernanGatta force-pushed the optee-generic-rpc-support branch 2 times, most recently from 5eb7ef1 to 31b6a60 Compare October 29, 2020 09:07
buffer = (void *)(uintptr_t)up->vals[n * 2];
size = up->vals[n * 2 + 1];
if (buffer && pt != TEE_PARAM_TYPE_MEMREF_OUTPUT) {
destination = PTR_ADD(mobj_va, mobj_offs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition may overflow or result in a buffer at least partially outside the mobj.

ocall_id = params[0].value.a;
ocall_up = (struct utee_params *)params[1].memref.buffer; /* User ptr */

res = ocall_check_parameters(&utc->uctx, ocall_up);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these checks are to be effective we need to keep ocall_up in OP-TEE Core memory, not the potentially less secure TA memory.

By the way, why not combine this function with ocall_compute_mobj_size()?

@jforissier
Copy link
Contributor

@HernanGatta ping?

@etienne-lms
Copy link
Contributor

Hello @HernanGatta and all,
Some info on this P-R series.
I am working on SCMI support in OP-TEE where OP-TEE Core exposes system resources to REE kernel: clocks, sensors, reset controller, and few more. One can have a look at the SCMI specification.
Using OCALLs, we could provision an OP-TEE thread for SCMI message exchange between REE kernel and TEE core. I plan to create some P-Rs for this based on your work, using only the REE/TEE kernels parts and maybe not the SHM parts, and get one's feedback.
Regards

@jforissier
Copy link
Contributor

@etienne-lms this PR was discussed in the last LOC meeting (link) and everyone agreed that it makes sense to revive this PR, so your contributions are welcome :) As mentioned in the notes, the difficult part may be related to some reference counting issue in the Linux driver patches so we concluded it would be good to start by clarifying this point.

@etienne-lms
Copy link
Contributor

Thanks for the link :)

checkpatch suggests using the BIT macro instead of a left shift.

Signed-off-by: Hernan Gatta <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Adds the ability for TAs to perform "Out Calls", or OCALLs. OCALLs allow
TAs to invoke commands on their corresponding CA in the same way that CAs
can invoke commands on TAs. Also, adds a new capability that reports
whether OP-TEE was built with OCALL support.

Signed-off-by: Hernan Gatta <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
@HernanGatta
Copy link
Author

Hello all, my apologies for the hiatus. I'm back and looking at this again, particularly the issues that were brought up regarding memory references. I'll report back with what I found. I'm glad to see functionality being implemented atop OCALLs :) .

@HernanGatta
Copy link
Author

Latest push is merely a rebase. Jens' comments still to be addressed.

ldebieve pushed a commit to STMicroelectronics/optee_os that referenced this pull request Jun 16, 2022
checkpatch suggests using the BIT macro instead of a left shift.

Signed-off-by: Hernan Gatta <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
[etienne: picked from OP-TEE#3673 series]
Signed-off-by: Etienne Carriere <[email protected]>
Change-Id: I4806c99bede5a31bf19b18eb77e86284767fd3fc
Reviewed-on: https://gerrit.st.com/c/mpu/oe/optee/optee_os/+/202482
Tested-by: Etienne CARRIERE <[email protected]>
Tested-by: Lionel DEBIEVE <[email protected]>
Reviewed-by: Etienne CARRIERE <[email protected]>
Reviewed-by: Lionel DEBIEVE <[email protected]>
ldebieve pushed a commit to STMicroelectronics/optee_os that referenced this pull request Jun 16, 2022
Adds the ability for Core to perform "Out Calls", or OCALLs. OCALLs
allow OP-TEE Core PTA services to invoke commands on their corresponding
client in the same way that client can invoke commands on TAs.

Adds a new capability that reports whether OP-TEE was built with OCALL
support.

Signed-off-by: Hernan Gatta <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
[etienne: picked from OP-TEE#3673 series]
[etienne: split CFG_OCALL into CFG_{CORE|USER}_OCALL, no libutee, no pta]
Signed-off-by: Etienne Carriere <[email protected]>
Change-Id: I410d8f5743473177f7091c6db0f9aea5fe4ce5bc
Reviewed-on: https://gerrit.st.com/c/mpu/oe/optee/optee_os/+/202483
Tested-by: Etienne CARRIERE <[email protected]>
Tested-by: Lionel DEBIEVE <[email protected]>
Reviewed-by: Etienne CARRIERE <[email protected]>
Reviewed-by: Lionel DEBIEVE <[email protected]>
arnout pushed a commit to arnout/optee_os that referenced this pull request Jan 6, 2023
checkpatch suggests using the BIT macro instead of a left shift.

Signed-off-by: Hernan Gatta <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
[etienne: picked from OP-TEE#3673 series]
Signed-off-by: Etienne Carriere <[email protected]>
Change-Id: I4806c99bede5a31bf19b18eb77e86284767fd3fc
Reviewed-on: https://gerrit.st.com/c/mpu/oe/optee/optee_os/+/202482
Tested-by: Etienne CARRIERE <[email protected]>
Tested-by: Lionel DEBIEVE <[email protected]>
Reviewed-by: Etienne CARRIERE <[email protected]>
Reviewed-by: Lionel DEBIEVE <[email protected]>
arnout pushed a commit to arnout/optee_os that referenced this pull request Jan 6, 2023
Adds the ability for Core to perform "Out Calls", or OCALLs. OCALLs
allow OP-TEE Core PTA services to invoke commands on their corresponding
client in the same way that client can invoke commands on TAs.

Adds a new capability that reports whether OP-TEE was built with OCALL
support.

Signed-off-by: Hernan Gatta <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
[etienne: picked from OP-TEE#3673 series]
[etienne: split CFG_OCALL into CFG_{CORE|USER}_OCALL, no libutee, no pta]
Signed-off-by: Etienne Carriere <[email protected]>
Change-Id: I410d8f5743473177f7091c6db0f9aea5fe4ce5bc
Reviewed-on: https://gerrit.st.com/c/mpu/oe/optee/optee_os/+/202483
Tested-by: Etienne CARRIERE <[email protected]>
Tested-by: Lionel DEBIEVE <[email protected]>
Reviewed-by: Etienne CARRIERE <[email protected]>
Reviewed-by: Lionel DEBIEVE <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants