mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Revise handling and testing of module-internal optimization options

Open gilles-peskine-arm opened this issue 11 months ago • 4 comments

We have several configuration options that tweak performance/memory/code-size/side-channel-resistance compromises of a specific part of the library, without affecting the functional behavior of the library. In particular, they do not affect the API or ABI. Traditionally these are not distinguished from other configuration options.

I think that in 4.0, we can simplify the handling and testing of options that don't affect the functional behavior, and that only affect a single code module:

  • Since they don't affect the behavior, application code doesn't need to #if on them, so they don't need to be in mbedtls_config.h or its crypto equivalents.
  • For testing, since they only affect a single code module, we don't need to run a whole test campaign, we only need to run one test suite with a minimal build.

https://github.com/Mbed-TLS/mbedtls/pull/8822 introduces some internal options to tweak optimizations in the SHA3 module. They're undocumented because we don't want to guarantee their stability in 3.6.x, but apart from the documentation aspect, I think they show a good way forward.

Documented options in 3.6 that could be in scope:

  • MBEDTLS_AES_ROM_TABLES, MBEDTLS_CAMELLIA_FEWER_TABLES
  • MBEDTLS_CAMELLIA_SMALL_MEMORY
  • MBEDTLS_SHA256_SMALLER, MBEDTLS_SHA512_SMALLER
  • MBEDTLS_SHA3_xxx_UNROLL if we decide to document them (and if not, the testing aspect should stay aligned)
  • Possibly, the hardware acceleration options (MBEDTLS_AESCE_C, MBEDTLS_AESNI_C, MBEDTLS_SHA256_USE_ARMV8_A_CRYPTO_IF_PRESENT, MBEDTLS_SHA256_USE_A64_CRYPTO_IF_PRESENT, MBEDTLS_SHA256_USE_ARMV8_A_CRYPTO_ONLY, MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT, MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY) could fit this as well? (AESCE and AESNI leak into gmc.)
  • and probably a few more: MBEDTLS_CTR_DRBG_USE_128_BIT_KEY, MBEDTLS_ECP_NIST_OPTIM, MBEDTLS_ECP_WITH_MPI_UINT, MBEDTLS_RSA_NO_CRT?

gilles-peskine-arm avatar Feb 28 '24 13:02 gilles-peskine-arm

This is something I've talked to @tom-cosgrove-arm about in the past. I suggested a general purpose MBEDTLS_SIZE_PERF_TRADEOFF type option, to give the user a single top-level tuning knob that they set to a number from, say, 1 - 5, to choose prioritisation between size / perf. My thinking was that this could automatically set up all of the fine-grained options, if they are unset, but the user could possibly have the option to over-ride them (e.g. set to 1 or 0). Tom was less keen on this approach.

daverodgman avatar Feb 28 '24 13:02 daverodgman

I don't like a general-purpose performance config option, since users might well want to optimise one area for code size and another for performance. I do like the style of options introduced #8822 - they are fine-grained and can be set on the command line either way without getting overridden (a "default on" option needs either edit to the config file to be able to turn it off)

tom-cosgrove-arm avatar Feb 28 '24 14:02 tom-cosgrove-arm

I also favor a general size/perf/security tradeoff (security: some users are not concerned about timing side channels, so ideally they shouldn't pay for our easily-bypassed countermeasures like blinding). But when an option is contained in a small block of code, its cost is much less.

gilles-peskine-arm avatar Feb 28 '24 15:02 gilles-peskine-arm

What needs to be finalized in terms of requirements for TF-PSA-Crypto 1.0:

  • Are we keeping all the high-granularity options as public options?
  • Are we making the private options (e.g. in SHA3) public?
  • Do we strictly enforce a category of options that are one-file-only? How does this fit with checking that all test cases are executed (which includes checking that all config options are tested both on and off)?
  • Naming pattern
  • Do we provide high-level options to optimize for speed/size/memory? (We can do that later in 4.x.)

gilles-peskine-arm avatar Aug 07 '24 17:08 gilles-peskine-arm