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

cancellation: Unmask inside of invoke command #732

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

c-auger
Copy link

@c-auger c-auger commented Feb 15, 2024

The specification seems to indicate (see #731)
that the cancellation mask is reset at the start of every entry point.
As a result, the TEE_UnmakCancellation() should not be called at the end of
TA_OpenSessionEntryPoint(), but at the start of TA_InvokeCommandEntryPoint().

Also, note that this patch should not break the current test.
Still, there is a test which could be written to test that unmasking cancellations
really does not affect subsequent entry points. There is no such test for now,
and if one were designed, then the current implementation would report a failure to this test,
until the implementation is fixed with respect to the Global Platform specification.
If you need such a test, I could consider writing one.

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.

cancellation: Unmask inside of invoke command

The specification seems to indicate (see #731)
that the cancellation mask is reset at the start of every entry point.
As a result, the TEE_UnmakCancellation() should not be called at the end of
TA_CreateEntryPoint(), but at the start of TA_OpenSessionEntryPoint().

s/TA_CreateEntryPoint()/TA_OpenSessionEntryPoint()/
s/TA_OpenSessionEntryPoint()/TA_InvokeCommandEntryPoint()/

With that fixed:

Reviewed-by: Jerome Forissier <[email protected]>

@jenswi-linaro
Copy link
Contributor

Also, note that this patch should not break the current test. Still, there is a test which could be written to test that unmasking cancellations really does not affect subsequent entry points. There is no such test for now, and if one were designed, then the current implementation would report a failure to this test, until the implementation is fixed with respect to the Global Platform specification. If you need such a test, I could consider writing one.

Sure, if you don't mind, please provide such a test in a separate PR. I can take care of the secure side of things.

@jforissier
Copy link
Contributor

Hi @c-auger, thanks for the patch!

Also, note that this patch should not break the current test. Still, there is a test which could be written to test that unmasking cancellations really does not affect subsequent entry points. There is no such test for now, and if one were designed, then the current implementation would report a failure to this test, until the implementation is fixed with respect to the Global Platform specification. If you need such a test, I could consider writing one.

That would be nice, please feel free to open a new PR. Then we can consider how to make the implementation compliant.

@jforissier
Copy link
Contributor

@c-auger we need a Signed-off-by: Your Name <your email> tag, please.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Commit header line should rather be "ta: os_test: Unmask cancellation from invoke command handler".
Acked-by: Etienne Carriere <[email protected]>

The specification seems to indicate (see OP-TEE#731)
that the cancellation mask is reset at the start of every entry point.
As a result, the TEE_UnmakCancellation() should not be called at the end of
TA_OpenSessionEntryPoint(), but at the start of TA_InvokeCommandEntryPoint().

Signed-off-by: Cedric Auger <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
@jforissier
Copy link
Contributor

@c-auger thanks!

@jforissier jforissier merged commit 3feb4fb into OP-TEE:master Feb 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants