-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Move TI hardware acceleration into Matter repo #9579
Conversation
Moved the SHA, AES, and ECJPAKE alt headers and source files into the Matter repository. Fixed an issue with the semantics of SHA256 context cloning. Changed the CHIPCryptoPALmbedTLS.cpp implementation to clone the temporary context back over the original context when done with the non-final finalization. Increased the SHA context opaque size in CHIPCryptoPAL.h to fit the TI driver accelerated SHA context. In future commits it may be possible to share a single SHA driver instance between all the contexts in mbedtls. This has the disadvantage of requiring a mutex for driver usage and reference counting. This would also increase processing time due to the thrashing of the internal digest buffer. But has the potential advantage that the SHA context can be copied directly with a memcpy.
Approved, but I second Andrei comment. The worst case should not be applied to kMAX_Hash_SHA256_Context_Size for all platforms. We should use a defined default value that a platform specific define can overrite. |
I was also very confused why the Crypto PAL component seemed to compile in the size of context objects and treated them as copy-able objects. But I was not quite confident in my understanding of the side-effects to OpenSSL to start messing with those files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cost of shared context everyone must use is too large.
Suggest:
- Add a CHIP_CONFIG variable for SHA256 Hardware Context size, used as a default define for
kMAX_Hash_SHA256_Context_Size
and set to previous value before this change. - Update TI examples to use CHIP_CONFIG_xxxx new define and override it to
(sizeof(unsigned int) * 76)
which would mean ONLY TI platform users would pay that cost.
The TI BLE Manager Impl incorrectly included a header that required cpp linkage.
The Hash_SHA256_stream object is now created on the stack and let to fall out of context at the end of a translation unit. This causes an issue if the resources are not freed. Update the mbedtls PAL to initialize and free the underlying SHA context structure.
The failures seem to be CIPD setup in the github pipeline. I don't think I can manually rerun those tests. Should I push a small change to kick them off again? |
Size increase report for "gn_qpg-example-build" from 8788f6f
Full report output
|
Size increase report for "esp32-example-build" from 8788f6f
Full report output
|
Size increase report for "nrfconnect-example-build" from 8788f6f
Full report output
|
Hi all, Thanks for the reviews here! Any further actions needed before merging this PR (by an authorized user)? Thanks, |
Problem
Fixes #9176
Change overview
Moved the SHA, AES, and ECJPAKE alt headers and source files into the Matter
repository. Fixed an issue with the semantics of SHA256 context cloning.
Changed the CHIPCryptoPALmbedTLS.cpp implementation to clone the temporary
context back over the original context when done with the non-final
finalization. Increased the SHA context opaque size in CHIPCryptoPAL.h to fit
the TI driver accelerated SHA context.
In future commits it may be possible to share a single SHA driver instance
between all the contexts in mbedtls. This has the disadvantage of requiring a
mutex for driver usage and reference counting. This would also increase
processing time due to the thrashing of the internal digest buffer. But has the
potential advantage that the SHA context can be copied directly with a memcpy.
Testing
Manually tested.