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

Mutex-protect mbedtls_hardware_poll #9532

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Conversation

kjbracey
Copy link
Contributor

Description

Like all HAL APIs, the calls in trng_api.h are not expected to be thread-safe.

All current accesses to the TRNG HAL are currently via mbedtls_hardware_poll. Mbed TLS does not currently serialise these calls itself, as MBEDTLS_THREADING_C is not enabled. But even if Mbed TLS's own accesses were serialised, there are other direct users of mbedtls_hardware_poll such as randLIB, that need to use direct calls due to lack of API to extract entropy from Mbed TLS.

As such it makes sense to treat mbedtls_hardware_poll as a de facto public Mbed OS API, akin to the C++ veneers on top of the HAL, and add a PlatformMutex there so that it is safe for multithreaded use.

Fixes #9513

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Like all HAL APIs, the calls in trng_api.h are not expected to
be thread-safe.

All current accesses to the TRNG HAL are currently via
`mbedtls_hardware_poll`.  Mbed TLS does not currently serialise these
calls itself, as `MBEDTLS_THREADING_C` is not enabled. But even if
Mbed TLS's own accesses were serialised, there are other direct
users of `mbedtls_hardware_poll` such as randLIB, that need to use
direct calls due to lack of API to extract entropy from Mbed TLS.

As such it makes sense to treat `mbedtls_hardware_poll` as a de facto
public Mbed OS API, akin to the C++ veneers on top of the HAL, and add a
PlatformMutex there so that it is safe for multithreaded use.
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 29, 2019

Thanks Kevin for fixing this one so quickly

@kjbracey
Copy link
Contributor Author

I felt a bit guilty because I'm one of the people who borrowed mbedtls_hardware_poll - for randLIB.

(And randLIB's lack of thread safety for its own state was bugging me the other day, although scrambling the PRNG state due to a thread race probably isn't the end of the world).

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 29, 2019

Confirmed fixing the issue reported, will schedule CI shortly

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 29, 2019

CI started

@kjbracey
Copy link
Contributor Author

Update note from #9513 - it is actually an undocumented Mbed TLS requirement that mbedtls_hardware_poll is thread-safe, regardless of its own MBEDTLS_THREADING_C mutexes. Those mutexes are per-entropy-context, so would not ever act as a lock on the system-singleton mbedtls_hardware_poll - it has to take care of itself.

@mbed-ci
Copy link

mbed-ci commented Jan 29, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

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