[mbedtls] Upgrade mbedTLS to version 3.4.0 #1256

kailun-qin · 2023-03-28T16:23:37Z

This commit introduces the following changes along with the upgrade of mbedTLS to version 3.4.0:

updating the sonames of mbedTLS-produced libs;
aligning all Gramine patches to conform with the new coding style of mbedTLS;
explicitly specifying the Gramine-provided secure zeroization function erase_memory() and updating it to a more performant version to cope with the updated mbedtls_platform_zeroize() detection logic;
dropping MBEDTLS_SHA224_C in the mbedTLS config which is now made independent from MBEDTLS_SHA256_C (it is actually not used in Gramine but was required to be present together with MBEDTLS_SHA256_C in the previous mbedTLS versions).

Fixes: #1255
Fixes: #1257

This change is

kailun-qin

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: )

subprojects/mbedtls-mbedtls-3.4.0.wrap line 4 at r1 (raw file):

directory = mbedtls-mbedtls-3.4.0
source_url = https://github.com/ARMmbed/mbedtls/archive/mbedtls-3.4.0.tar.gz
source_fallback_url = https://packages.gramineproject.io/distfiles/mbedtls-3.4.0.tar.gz

@woju: Please kindly help mirror this file. Thanks!

I'll test against it afterwards.

woju

Reviewed 8 of 10 files at r1, 3 of 3 files at r2.
Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin)

-- commits line 2 at r2:
Commit message lacks description of the changes that were done together with this update. I've noticed at least:

that the AES-NI patch was dropped wrt. upstream fix (and was the upstream fix reviewed to be sufficient for us?)
sonames were changed
zeroize
sha224

Also please add "Fixes: #1255" either to PR description or here to the commit.

subprojects/mbedtls-mbedtls-3.4.0.wrap line 4 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

@woju: Please kindly help mirror this file. Thanks!

I'll test against it afterwards.

Done.

subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 38 at r2 (raw file):

#define MBEDTLS_PLATFORM_C
#define MBEDTLS_RSA_C
#define MBEDTLS_SHA224_C

Why this was removed, and if there's a reason, why isn't it written either it commit message, or at least in the PR?

kailun-qin

Reviewable status: 8 of 12 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @woju)

-- commits line 2 at r2:

From #1255:
mbedtls 3.4.0 was released earlier today: https://github.com/Mbed-TLS/mbedtls/releases/tag/v3.4.0
It has fixed a vulnerability which we have mitigated with the patch: https://github.com/gramineproject/gramine/blob/43101b8fa3fd4c1cd6c9304c2094ce009627efb3/subprojects/packagefiles/mbedtls/enforce-aes-ni.patch

I saw your above comment when I submitted the first version of PR. However, after taking a deeper look at the changes in mbedtls, I don't think it's sufficient for us as it still does not enforce AES-NI at run time. Pls kindly double check https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aes.c#L581-L585 and https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aesni.c for details.

that the AES-NI patch was dropped wrt. upstream fix (and was the upstream fix reviewed to be sufficient for us?)

So dropped this change.

Also please add "Fixes: #1255" either to PR description or here to the commit.

Done and updated the PR description.

subprojects/mbedtls-mbedtls-3.4.0.wrap line 4 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Done.

Tested it manually. It works fine.

subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 38 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Why this was removed, and if there's a reason, why isn't it written either it commit message, or at least in the PR?

We actually did not use MBEDTLS_SHA224_C at all. Updated the commit message.

kailun-qin

Reviewable status: 8 of 12 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @woju)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

/* mbedTLS library will use this implementation of zeroizing a block of memory, see
 * https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/include/mbedtls/mbedtls_config.h#L3890 */
void mbedtls_platform_zeroize(void* buf, size_t len) {

In fact I'm not sure about this - previously we were implicitly using simple memset(): https://github.com/Mbed-TLS/mbedtls/blob/v3.3.0/library/platform_util.c#L65. Do we require this in mbedTLS PAL to securely scrub sensitive memory bufs?

woju

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

-- commits line 2 at r2:

Previously, kailun-qin (Kailun Qin) wrote…

From #1255:
mbedtls 3.4.0 was released earlier today: https://github.com/Mbed-TLS/mbedtls/releases/tag/v3.4.0
It has fixed a vulnerability which we have mitigated with the patch: https://github.com/gramineproject/gramine/blob/43101b8fa3fd4c1cd6c9304c2094ce009627efb3/subprojects/packagefiles/mbedtls/enforce-aes-ni.patch

I saw your above comment when I submitted the first version of PR. However, after taking a deeper look at the changes in mbedtls, I don't think it's sufficient for us as it still does not enforce AES-NI at run time. Pls kindly double check https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aes.c#L581-L585 and https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aesni.c for details.

that the AES-NI patch was dropped wrt. upstream fix (and was the upstream fix reviewed to be sufficient for us?)

So dropped this change.

Also please add "Fixes: #1255" either to PR description or here to the commit.

Done and updated the PR description.

Right. I think if you can fail mbedtsl_aesni_has_support (https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aesni.c#L45), then you can still fallthrough into software AES implementation.

@mkow ^ can you read those mbedtls fixes? You did this patch.

If so, do we have a reason to update to 3.4.0? If we don't, then we probably shouldn't, because this change breaks ABI (sonames) for no good reason.

kailun-qin

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

-- commits line 2 at r2:

If so, do we have a reason to update to 3.4.0? If we don't, then we probably shouldn't, because this change breaks ABI (sonames) for no good reason.

Pesonally, I think we still have some reasons to update to 3.4.0, for instance:

It still introduced some security feautre updates (e.g., zeroize SSL cache entries when they are freed) along with normal bug fixes (e.g., integer overflow), as the update is recommaned by mbedTLS.
SHA224_C is now independent from SHA256_C. This helps in saving code size and resolving confusions (e.g., discussed somewhere here in [mbedtls,tools/sgx,CI-Examples,common] Upgrade mbedTLS to version 3.2.1 #796 (review)) since we do not use it at all in Gramine.
It changed the default MBEDTLS_ECP_WINDOW_SIZE from 6 to 2. As stated by mbedTLS, the correlation between this define and RSA decryption performance has changed lately due to security fixes. To fix the performance degradation when using default values the window was reduced from 6 to 2, a value that gives the best or close to best results when tested on Cortex-M4 and Intel i7.
We made some decisions previously to keep up to the latest version for mbedTLS (as discussed e.g., in [Pal,common,CI-Examples] Update mbedTLS from version 2.26.0 to 3.1.0 #426 (review) and [Pal,common,CI-Examples] Update mbedTLS from version 2.26.0 to 3.1.0 #426 (review)).

Anyway, I'm open to it.

dimakuv

Reviewed 9 of 10 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin and @mkow)

-- commits line 2 at r2:

Previously, kailun-qin (Kailun Qin) wrote…

If so, do we have a reason to update to 3.4.0? If we don't, then we probably shouldn't, because this change breaks ABI (sonames) for no good reason.

Pesonally, I think we still have some reasons to update to 3.4.0, for instance:

It still introduced some security feautre updates (e.g., zeroize SSL cache entries when they are freed) along with normal bug fixes (e.g., integer overflow), as the update is recommaned by mbedTLS.

SHA224_C is now independent from SHA256_C. This helps in saving code size and resolving confusions (e.g., discussed somewhere here in [mbedtls,tools/sgx,CI-Examples,common] Upgrade mbedTLS to version 3.2.1 #796 (review)) since we do not use it at all in Gramine.

It changed the default MBEDTLS_ECP_WINDOW_SIZE from 6 to 2. As stated by mbedTLS, the correlation between this define and RSA decryption performance has changed lately due to security fixes. To fix the performance degradation when using default values the window was reduced from 6 to 2, a value that gives the best or close to best results when tested on Cortex-M4 and Intel i7.

We made some decisions previously to keep up to the latest version for mbedTLS (as discussed e.g., in [Pal,common,CI-Examples] Update mbedTLS from version 2.26.0 to 3.1.0 #426 (review) and [Pal,common,CI-Examples] Update mbedTLS from version 2.26.0 to 3.1.0 #426 (review)).

Anyway, I'm open to it.

I also think that we should keep mbedTLS to the latest version. Like, why not? Clearly a newer version has bug fixes, so we should use it.

-- commits line 4 at r4:
upgradation -> upgrade

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

In fact I'm not sure about this - previously we were implicitly using simple memset(): https://github.com/Mbed-TLS/mbedtls/blob/v3.3.0/library/platform_util.c#L65. Do we require this in mbedTLS PAL to securely scrub sensitive memory bufs?

I vote for removing this. Let's use mbedTLS's default implementation, which should survive compiler optimizations and is also performant (because it uses memset()).

The erase_memory() function that we have (

gramine/common/include/api.h

Lines 403 to 410 in 0119fd2

    
           /* Scrub sensitive memory bufs (memset can be optimized away and memset_s is not available in PAL). 
        
            * FIXME: This implementation is inefficient (and used in perf-critical functions). 
        
            * TODO:  Is this really needed? Intel SGX SDK uses similar function as "defense in depth". */ 
        
           static inline void erase_memory(void* buffer, size_t size) { 
        
               volatile unsigned char* p = buffer; 
        
               while (size--) 
        
                   *p++ = 0; 
        
           }

) is highly dubious and not performant at all. So I wouldn't want to use it in any way.

I created an issue about erase_memory() here: #1257

subprojects/packagefiles/mbedtls/fcntl.patch line 27 at r4 (raw file):

      * Never return 'WOULD BLOCK' on a blocking socket
      */
-    if ((fcntl(ctx->fd, F_GETFL) & O_NONBLOCK) != O_NONBLOCK) {

Wow, mbedTLS finally uses adequate formatting :) I was always surprised when I looked at their code, how unreadable this formatting was.

subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 37 at r4 (raw file):

#define MBEDTLS_PKCS1_V15
#define MBEDTLS_PLATFORM_C
#define MBEDTLS_PLATFORM_ZEROIZE_ALT

I'd remove this, as discussed in the other comment

kailun-qin

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)

-- commits line 4 at r4:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

upgradation -> upgrade

Done.

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I vote for removing this. Let's use mbedTLS's default implementation, which should survive compiler optimizations and is also performant (because it uses memset()).

The erase_memory() function that we have (

gramine/common/include/api.h

Lines 403 to 410 in 0119fd2

/* Scrub sensitive memory bufs (memset can be optimized away and memset_s is not available in PAL).

* FIXME: This implementation is inefficient (and used in perf-critical functions).

* TODO: Is this really needed? Intel SGX SDK uses similar function as "defense in depth". */

static inline void erase_memory(void* buffer, size_t size) {

volatile unsigned char* p = buffer;

while (size--)

*p++ = 0;

}

) is highly dubious and not performant at all. So I wouldn't want to use it in any way.

I created an issue about erase_memory() here: #1257

Switched to mbedTLS's built-in implementation using the trick of a volatile function pointer to memset() instead of our erase_memory.

subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 37 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'd remove this, as discussed in the other comment

The reason I explicitly set this is due to that mbedTLS 3.4.0 has added a logic of platform-provided secure memset auto detection at compile time. Pls kindly see https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/platform_util.c#L52-L57 and https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/platform_util.c#L69-L130.

However, at the linking phase, we may then encounter some issues like

/usr/local/bin/ld: subprojects/mbedtls-mbedtls-3.4.0/libmbedcrypto_pal.a(platform_util.o): in function `mbedtls_platform_zeroize':
platform_util.c:(.text+0xd8): undefined reference to `__explicit_bzero_chk'

I stated the reason in the PR description and now in the comment as well.

dimakuv

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)

-- commits line 32 at r5:
space before in the mbedTLS ...

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Switched to mbedTLS's built-in implementation using the trick of a volatile function pointer to memset() instead of our erase_memory.

Thanks, now it makes sense!

subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 37 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

The reason I explicitly set this is due to that mbedTLS 3.4.0 has added a logic of platform-provided secure memset auto detection at compile time. Pls kindly see https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/platform_util.c#L52-L57 and https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/platform_util.c#L69-L130.

However, at the linking phase, we may then encounter some issues like
/usr/local/bin/ld: subprojects/mbedtls-mbedtls-3.4.0/libmbedcrypto_pal.a(platform_util.o): in function `mbedtls_platform_zeroize':
platform_util.c:(.text+0xd8): undefined reference to `__explicit_bzero_chk'
I stated the reason in the PR description and now in the comment as well.

Thanks for the explanation.

kailun-qin

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

-- commits line 32 at r5:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

space before in the mbedTLS ...

Fixed, thanks!

woju

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

-- commits line 2 at r2:
OK, this seems to be reasonable. Unblocking, but I'd still be grateful if @mkow read through this upstream (non-)fix.

Like, why not?

Because this is incompatible update: 2 out of 3 mbed*_gramine.so libraries will change SONAME? This means that if someone uses our distribution of mbedtls (and users should at least because of our mitigation of AES-NI problem), s/he'll need to at least recompile the app. Not fun, but obviously can be done if the reason is compelling.

dimakuv

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @mkow)

-- commits line 2 at r2:

This means that if someone uses our distribution of mbedtls (and users should at least because of our mitigation of AES-NI problem), s/he'll need to at least recompile the app. Not fun, but obviously can be done if the reason is compelling.

Do SONAMEs really prevent the loader to load the newer-version shared lib? Ok, then I agree that it's bad...

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @mkow)

-- commits line 2 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This means that if someone uses our distribution of mbedtls (and users should at least because of our mitigation of AES-NI problem), s/he'll need to at least recompile the app. Not fun, but obviously can be done if the reason is compelling.

Do SONAMEs really prevent the loader to load the newer-version shared lib? Ok, then I agree that it's bad...

UPDATE: I think SONAME is irrelevant during runtime (when the loader tries to find the shared library)? Also, this SO answer seems to say the same: https://stackoverflow.com/questions/12637841/what-is-the-soname-option-for-building-shared-libraries-for

woju

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @mkow)

-- commits line 2 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

UPDATE: I think SONAME is irrelevant during runtime (when the loader tries to find the shared library)? Also, this SO answer seems to say the same: https://stackoverflow.com/questions/12637841/what-is-the-soname-option-for-building-shared-libraries-for

Yes, this will prevent loader from loading libraries. I didn't mention NEEDED for simplicity, but yes, to be precise it's NEEDED that will prevent loading. To be even more precise: the lookup will fail, because there will be no file matching NEEDED (the new one won't be matched because filename is different), and the filename is different to match SONAME.

% objdump -p CI-Examples/ra-tls-mbedtls/client | fgrep NEEDED
  NEEDED               libdl.so.2
  NEEDED               libmbedcrypto_gramine.so.13
  NEEDED               libmbedtls_gramine.so.19
  NEEDED               libmbedx509_gramine.so.4
  NEEDED               libc.so.6
% objdump -p CI-Examples/ra-tls-mbedtls/server | fgrep NEEDED
  NEEDED               libdl.so.2
  NEEDED               libmbedcrypto_gramine.so.13
  NEEDED               libmbedtls_gramine.so.19
  NEEDED               libmbedx509_gramine.so.4
  NEEDED               libc.so.6

The SONAME was probably changed for a reason, so just changing the filename is not a solution. Those really are different, incompatible libraries, and what you're trying to do is to ship a breaking change. Again, that's OK if there's compelling reason. We just need to properly warn the users, that's why I insisted on at least a mention in the commit.

mkow

Reviewed 8 of 10 files at r1, 3 of 3 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @woju)

-- commits line 2 at r2:

I saw your above comment when I submitted the first version of PR. However, after taking a deeper look at the changes in mbedtls, I don't think it's sufficient for us as it still does not enforce AES-NI at run time. Pls kindly double check https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aes.c#L581-L585 and https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aesni.c for details.

I see some changes there, but none of them seem to be related to the problem. It still looks vulnerable.

Also please add "Fixes: #1255" either to PR description or here to the commit.

@woju: we only add these to PR descriptions.

Also +1 from my side to updating it even if it doesn't fix the AES-NI problem. I don't think we have too many users which would be affected by this and the update includes fixes related to security and performance.

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Thanks, now it makes sense!

I'm against this change, see #1257.

subprojects/packagefiles/mbedtls/fcntl.patch line 27 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Wow, mbedTLS finally uses adequate formatting :) I was always surprised when I looked at their code, how unreadable this formatting was.

Yeah. But it was still nothing compared to GNU style :)

kailun-qin

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'm against this change, see #1257.

For this change here (orthogonal to replace erase_memory() with more performant alternative in #1257), since we'll anyway need mbedtls_platform_zeroize() to skip mbedTLS's auto detection, I'll just remove the volatile function pointer trick to avoid any potential misleading and simply use our memset instead. WDYT?

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "squash! " found in commit messages' one-liners (waiting on @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

I'll just remove the volatile function pointer trick to avoid any potential misleading and simply use our memset instead. WDYT?

Yes please.

kailun-qin

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'll just remove the volatile function pointer trick to avoid any potential misleading and simply use our memset instead. WDYT?

Yes please.

Done.

dimakuv

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

mkow

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Done.

But why memset() instead of erase_memory()? memset() is very likely to be removed by the optimizer.
We'll take care of its performance when resolving #1257.

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But why memset() instead of erase_memory()? memset() is very likely to be removed by the optimizer.
We'll take care of its performance when resolving #1257.

memset() is written in volatile inline asm in Gramine for x86-64 (as @kailun-qin mentioned in #1257):

gramine/common/src/string/memset.c

Line 27 in 25ddc0e

    
           __asm__ volatile("rep stosb" : "+D"(d), "+c"(count) : "a"((uint8_t)ch) : "cc", "memory");

So it's fine. I would add a comment above memset invocation that it's written in pure asm so won't be optimized away (and say smth like see common/src/string/memset.c)

kailun-qin

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

memset() is written in volatile inline asm in Gramine for x86-64 (as @kailun-qin mentioned in #1257):

gramine/common/src/string/memset.c

Line 27 in 25ddc0e

__asm__ volatile("rep stosb" : "+D"(d), "+c"(count) : "a"((uint8_t)ch) : "cc", "memory");

So it's fine. I would add a comment above memset invocation that it's written in pure asm so won't be optimized away (and say smth like see common/src/string/memset.c)

Added a comment, thanks!

dimakuv

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

mkow

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

memset() is written in volatile inline asm in Gramine for x86-64 [...] So it's fine.

It's not, see #1257 (comment).

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

memset() is written in volatile inline asm in Gramine for x86-64 [...] So it's fine.

It's not, see #1257 (comment).

Ok, my last attempt: just directly insert the inline asm: https://github.com/gramineproject/gramine/blob/25ddc0effbf84c4a6b1a2d4bce93f3113f1ec04b/common/src/string/memset.c#L27?plain=1

kailun-qin

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Ok, my last attempt: just directly insert the inline asm: https://github.com/gramineproject/gramine/blob/25ddc0effbf84c4a6b1a2d4bce93f3113f1ec04b/common/src/string/memset.c#L27?plain=1

Shouldn't we use erase_memory() here and insert the inline asm in erase_memory() (i.e., resolve #1257)? Just as @mkow suggested above.

kailun-qin

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

-- commits line 2 at r2:

Previously, mkow (Michał Kowalczyk) wrote…

I saw your above comment when I submitted the first version of PR. However, after taking a deeper look at the changes in mbedtls, I don't think it's sufficient for us as it still does not enforce AES-NI at run time. Pls kindly double check https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aes.c#L581-L585 and https://github.com/Mbed-TLS/mbedtls/blob/v3.4.0/library/aesni.c for details.

I see some changes there, but none of them seem to be related to the problem. It still looks vulnerable.

Also please add "Fixes: #1255" either to PR description or here to the commit.

@woju: we only add these to PR descriptions.

Also +1 from my side to updating it even if it doesn't fix the AES-NI problem. I don't think we have too many users which would be affected by this and the update includes fixes related to security and performance.

FYI, a PR WIP: Mbed-TLS/mbedtls#7384 may serve our purpose of the AES-NI patch.

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Ok, my last attempt: just directly insert the inline asm: https://github.com/gramineproject/gramine/blob/25ddc0effbf84c4a6b1a2d4bce93f3113f1ec04b/common/src/string/memset.c#L27?plain=1

Shouldn't we use erase_memory() here and insert the inline asm in erase_memory() (i.e., resolve #1257)? Just as @mkow suggested above.

Ah, yes, please! Even better, we'll solve two problems then.

kailun-qin

Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

common/src/crypto/adapters/mbedtls_adapter.c line 512 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Ah, yes, please! Even better, we'll solve two problems then.

Done.

mkow

Reviewed 1 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)

-- commits line 2 at r2:

Previously, kailun-qin (Kailun Qin) wrote…

FYI, a PR WIP: Mbed-TLS/mbedtls#7384 may serve our purpose of the AES-NI patch.

Yup, looks good, we will be able to drop my patch and replace it with this additional define during mbedtls build.

common/include/api.h line 408 at r10 (raw file):

 * we're safe against being optimized away. */
static inline void erase_memory(void* buffer, size_t size) {
    char* buf = buffer;

What's the point of this?

common/include/api.h line 409 at r10 (raw file):

static inline void erase_memory(void* buffer, size_t size) {
    char* buf = buffer;
    __asm__ volatile("rep stosb" : "+D"(buf), "+c"(size) : "a"((uint8_t)0) : "cc", "memory");

This needs to be wrapped in an #ifdef-#else-#error, like other places where we use architecture-specific stuff.

common/src/crypto/adapters/mbedtls_adapter.c line 518 at r10 (raw file):

    assert(len == 0 || buf != NULL);

    if (len > 0) {

This IF doesn't seem to do anything.

kailun-qin

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

common/include/api.h line 408 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's the point of this?

It was simply made for casting purpose. Otherwise casting in the inline asm and building w/ clang will fail with "invalid use of a cast in a inline asm context requiring an l-value". Anyway, I think it's fine that we remove the casting.

common/include/api.h line 409 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This needs to be wrapped in an #ifdef-#else-#error, like other places where we use architecture-specific stuff.

Done.

common/src/crypto/adapters/mbedtls_adapter.c line 518 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This IF doesn't seem to do anything.

Right, removed.

mkow

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

common/include/api.h line 408 at r10 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

It was simply made for casting purpose. Otherwise casting in the inline asm and building w/ clang will fail with "invalid use of a cast in a inline asm context requiring an l-value". Anyway, I think it's fine that we remove the casting.

Sounds weird, buffer is already an l-value, why would you need a cast/another variable there?
Anyways, looks good now.

dimakuv

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

woju

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

This commit introduces the following changes along with the upgrade of mbedTLS to version 3.4.0: * updating the sonames of mbedTLS-produced libs; * aligning all Gramine patches to conform with the new coding style of mbedTLS; * explicitly specifying the Gramine-provided secure zeroization function `erase_memory()` and updating it to a more performant version to cope with the updated `mbedtls_platform_zeroize()` detection logic; * dropping `MBEDTLS_SHA224_C` in the mbedTLS config which is now made independent from `MBEDTLS_SHA256_C` (it is actually not used in Gramine but was required to be present together with `MBEDTLS_SHA256_C` in the previous mbedTLS versions). Signed-off-by: Kailun Qin <[email protected]>

dimakuv

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

mkow

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

woju

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

kailun-qin commented Mar 28, 2023

View reviewed changes

kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.4.0 branch from bf0159c to 0b3fa68 Compare March 28, 2023 16:36

woju requested changes Mar 28, 2023

View reviewed changes

kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.4.0 branch 2 times, most recently from 8c68c7e to 0dac962 Compare March 28, 2023 18:22

kailun-qin commented Mar 28, 2023

View reviewed changes

woju requested changes Mar 28, 2023

View reviewed changes

kailun-qin commented Mar 29, 2023

View reviewed changes

dimakuv suggested changes Mar 29, 2023

View reviewed changes

kailun-qin commented Mar 29, 2023

View reviewed changes

dimakuv suggested changes Mar 29, 2023

View reviewed changes

kailun-qin commented Mar 29, 2023

View reviewed changes

woju reviewed Mar 29, 2023

View reviewed changes

dimakuv approved these changes Mar 29, 2023

View reviewed changes

woju reviewed Mar 29, 2023

View reviewed changes

mkow requested changes Mar 30, 2023

View reviewed changes

kailun-qin commented Mar 30, 2023

View reviewed changes

dimakuv previously approved these changes Mar 30, 2023

View reviewed changes

kailun-qin dismissed dimakuv’s stale review via a759077 March 30, 2023 07:44

kailun-qin commented Mar 30, 2023

View reviewed changes

dimakuv approved these changes Mar 30, 2023

View reviewed changes

mkow requested changes Mar 30, 2023

View reviewed changes

dimakuv previously approved these changes Mar 31, 2023

View reviewed changes

woju mentioned this pull request Mar 31, 2023

SGX loader is not statically linked #1261

Open

kailun-qin dismissed dimakuv’s stale review via 7085574 April 3, 2023 03:36

kailun-qin commented Apr 3, 2023

View reviewed changes

dimakuv approved these changes Apr 3, 2023

View reviewed changes

mkow requested changes Apr 3, 2023

View reviewed changes

dimakuv suggested changes Apr 3, 2023

View reviewed changes

kailun-qin commented Apr 4, 2023

View reviewed changes

dimakuv suggested changes Apr 4, 2023

View reviewed changes

kailun-qin commented Apr 4, 2023

View reviewed changes

mkow requested changes Apr 4, 2023

View reviewed changes

kailun-qin commented Apr 4, 2023

View reviewed changes

mkow previously approved these changes Apr 4, 2023

View reviewed changes

dimakuv previously approved these changes Apr 4, 2023

View reviewed changes

woju previously approved these changes Apr 4, 2023

View reviewed changes

dimakuv dismissed stale reviews from woju, mkow, and themself via ea047eb April 4, 2023 17:39

dimakuv force-pushed the kailun-qin/upgrade-mbedtls-3.4.0 branch from a17c29b to ea047eb Compare April 4, 2023 17:39

dimakuv approved these changes Apr 4, 2023

View reviewed changes

mkow approved these changes Apr 4, 2023

View reviewed changes

woju approved these changes Apr 4, 2023

View reviewed changes

mkow merged commit ea047eb into gramineproject:master Apr 4, 2023

	/* Scrub sensitive memory bufs (memset can be optimized away and memset_s is not available in PAL).
	* FIXME: This implementation is inefficient (and used in perf-critical functions).
	* TODO: Is this really needed? Intel SGX SDK uses similar function as "defense in depth". */
	static inline void erase_memory(void* buffer, size_t size) {
	volatile unsigned char* p = buffer;
	while (size--)
	*p++ = 0;
	}

[mbedtls] Upgrade mbedTLS to version 3.4.0 #1256

[mbedtls] Upgrade mbedTLS to version 3.4.0 #1256

Conversation

kailun-qin commented Mar 28, 2023 • edited Loading

kailun-qin left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

kailun-qin commented Mar 28, 2023 •

edited

Loading