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

core: crypto: libtomcrypt: fix LTC_CLEAN_STACK bug #3102

Merged
merged 1 commit into from
Jul 1, 2019

Conversation

b49020
Copy link
Contributor

@b49020 b49020 commented Jun 27, 2019

LTC_CLEAN_STACK uses burn_stack() API that uses a recursive call which
leads to approx. double the size of stack cleaned than expected on ARM64.
So this causes stack overflow corrupting canaries in case we perform a
SHA512 hash operation which utilizes maximum stack as compared to other
libtomcrypt APIs. So get rid of this recursive call via using variable
length array to clean stack.

Also, convert zeromem() API as a wrapper to call memzero_explicit().

Fixes: ad56511 ("core: crypto: libtomcrypt: enable LTC_CLEAN_STACK")
Suggested-by: Daniel Thompson [email protected]
Signed-off-by: Sumit Garg [email protected]

@jforissier
Copy link
Contributor

jforissier commented Jul 1, 2019

As I said previously [1] this way of scrubbing the stack is wrong IMO, but the code introduced by this PR is clearly much better since it is much less likely to cause a stack overflow, as per your analysis in [2].
Would you mind clarifying why the current code causes a stack overflow? I suggest: "...on ARM64, because it consumes stack space in 32-byte chunks and assumes only buf is pushed onto the stack while ignoring any other data such as lr, fp, etc.". Or something like that.

With the commit text clarified:

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

[1] #3064 (comment)
[2] #3064 (comment)

@b49020
Copy link
Contributor Author

b49020 commented Jul 1, 2019

As I said previously [1] this way of scrubbing the stack is wrong IMO, but the code introduced by this PR is clearly much better since it is much less likely to cause a stack overflow, as per your analysis in [2].

Agree, burn_stack() api is something that people agree in upstream [1] to get rid off. But the correct approach to clean stack is still under discussion in upstream [1]. So for OP-TEE 3.6.0 release we can live with this least intrusive fix for burn_stack() api.

Would you mind clarifying why the current code causes a stack overflow? I suggest: "...on ARM64, because it consumes stack space in 32-byte chunks and assumes only buf is pushed onto the stack while ignoring any other data such as lr, fp, etc.".

Sure, will clarify commit text.

[1] libtom/libtomcrypt#486

LTC_CLEAN_STACK uses burn_stack() API that uses a recursive call which
leads to approx. double the size of stack cleaned than expected on ARM64,
because it consumes stack space in 32-byte chunks and assumes only buf
is pushed onto the stack while ignoring any other data such as lr, fp,
etc.. This causes stack overflow corrupting canaries in case we perform
a SHA512 hash operation which utilizes maximum stack as compared to other
libtomcrypt APIs. So get rid of this recursive call via using variable
length array to clean stack.

Also, convert zeromem() API as a wrapper to call memzero_explicit().

Fixes: ad56511 ("core: crypto: libtomcrypt: enable LTC_CLEAN_STACK")
Suggested-by: Daniel Thompson <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
@b49020
Copy link
Contributor Author

b49020 commented Jul 1, 2019

Updated PR.

@jforissier jforissier merged commit 03121b2 into OP-TEE:master Jul 1, 2019
alexjba added a commit to status-im/status-go that referenced this pull request May 8, 2023
Implements a cross-platform version of OP-TEE/optee_os#3102

1. Remove recursion
2. Use memset instead of while loop
alexjba added a commit to status-im/go-sqlcipher that referenced this pull request May 9, 2023
Fixing status-im/status-desktop#10572

Implements a cross-platform version of OP-TEE/optee_os#3102

Remove recursion
Use memset instead of while loop
A description to understand introduced changes without reading the code.

zeromem weights about 50% of the total CPU time on M1 Macs and seems to be major performance offender.
It is used to clear the stack when using variables with sensitive information.
alexjba added a commit to status-im/status-go that referenced this pull request May 10, 2023
Implements a cross-platform version of OP-TEE/optee_os#3102

1. Remove recursion
2. Use memset instead of while loop
alexjba added a commit to status-im/status-go that referenced this pull request May 12, 2023
Implements a cross-platform version of OP-TEE/optee_os#3102

1. Remove recursion
2. Use memset instead of while loop
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.

2 participants