Fix hmac_drbg failure in benchmark, with threading #1758
Conversation
RonEld
added
bug
mbed TLS team
needs-review
There was a problem hiding this comment.
I briefly looked at this PR and left a question regarding API usage. Also, please note that this problem also happens in the (mbed-os-example-tls repo](https://github.com/ARMmbed/mbed-os-example-tls/blob/master/benchmark/main.cpp#L689).
programs/test/benchmark.c
Outdated
| @@ -648,7 +648,6 @@ int main( int argc, char *argv[] ) | |||
| TIME_AND_TSC( "HMAC_DRBG SHA-1 (NOPR)", | |||
| if( mbedtls_hmac_drbg_random( &hmac_drbg, buf, BUFSIZE ) != 0 ) | |||
| mbedtls_exit(1) ); | |||
| mbedtls_hmac_drbg_free( &hmac_drbg ); | |||
There was a problem hiding this comment.
I definitely think that calling mbedtls_hmac_drbg_free() here is not correct, but I am not sure whether just removing it should work in all cases. That is, there are probably no issues with just calling mbedtls_hmac_drbg_seed() repeatedly when using the default software code, but I am not sure what happens if, for example, the implementation is a hardware accelerator. Are we expected to call init->seed->seed->free or is it necessary to do init->seed->free->init->seed->free?
Also, I was trying to check what happens inside mbedtls_hmac_drbg_seed(), so I looked up the documentation for mbedtls_md_setup() which is called internally from seed and found the following:
* It should be called after mbedtls_md_init() or
* mbedtls_md_free(). Makes it necessary to call
* mbedtls_md_free() later.
So according to this, it not necessary to call init at all when using the MD abstraction?
There was a problem hiding this comment.
@andresag01 if by HW acceleration you mean the HW entropy source, then I don't see a problem. Calling seed the second time is similar to reseeding. The HW should be stateless and should return the desirted entropy whenever asked.
|
@andresag01 I agree that the |
|
As far as I remember, we had some pending changes to Mbed TLS to make sure that the Mbed TLS multithreading abstraction used through MBEDTLS_THREADING_C had an implementation in Mbed OS, but I do not think it has been merged because some changes are needed to the Mbed TLS APIs (e.g. #1131) that have not been finalized. So I think for now the mbed-os-example-tls will work, but it would still be good to fix it there as well or to raise an issue. |
ChangeLog
Outdated
| = mbed TLS x.xx.x branch released xxxx-xx-xx | ||
|
|
||
| Bugfix | ||
| * Fix efailure in hmac_drbg in the benchmark sample application, when |
There was a problem hiding this comment.
'efailure?' - typo surely?
simonbutcher
added
needs-work
and removed
needs-review
RonEld
added
the
needs-review
|
@sbutcher-arm I fixed the typo I will rebase to resolve conflicts once the PR is reviewed. |
|
@RonEld - I've approved it, but I think you should go ahead and rebase now, else you'll need another review now, and another two after rebasing, which will just slow things down. |
Remove redunadnat calls to `hmac_drbg_free()` between seeding operations, which make the mutex invalid. Fixes Mbed-TLS#1095
|
@sbutcher-arm I have rebased and resolved conflicts. |
|
Backported to branches 2.1 and 2.7 in #1898 and #1899 cc @andresag01 @sbutcher-arm |
There was a problem hiding this comment.
Looks good to me - however, it needs back porting to the Mbed OS example as you can see here.
simonbutcher
added
approved
|
@RonEld - Can you please backport this to the equivalent Mbed OS example? |
|
@sbutcher-arm @RonEld: Please note that the PR ARMmbed/mbed-os-example-tls#194 already addresses these problems. |
|
Removing the "needs backports" label, as all backports have been fully approved |
RonEld
removed
the
needs-backports
Description
Remove redunadnat calls to
hmac_drbg_free()between seeding operations,which make the mutex invalid. Fixes #1095
Status
READY
Requires Backporting
Yes
Which branch?
mbedtls-2.1
mbedtls-2.7
Migrations
NO
Todos
Steps to test or reproduce
Outline the steps to test or reproduce the PR here.