Document and fix handling of MBEDTLS_PLATFORM_C and platform.h

[gilles-peskine-arm]

Overview

Most of the C source files in Mbed TLS use the following idiom:

#if defined(MBEDTLS_PLATFORM_C)
#include "mbedtls/platform.h"
#else
#define mbedtls_stdfunc stdfunc … // define aliases for standard library functions
#endif /* MBEDTLS_PLATFORM_C */

I think this is both overly complex and wrong, though I may be missing something. My proposal: get rid of this idiom completely and replace it by an unconditional #include "mbedtls/platform.h".

Rationale for the current behavior

I'm not sure. Including an extra header file should not break working code.

Maybe the intent is that you can copy a few Mbed TLS source files into your application, and not bother copying platform.h if you haven't enabled MBEDTLS_PLATFORM_C? For example copy just aes.c and aes.h if you want AES and nothing else? While this is blessed in the Mbed TLS philosophy document, it mostly doesn't really work in practice. Since I joined the project in 2017, I only remember seeing a couple of support requests of this kind, and we've mostly not responded to those requests. The specific example of aes.{h,c}-and-nothing-else given in the philosophy document hasn't worked since 2018 when https://github.com/Mbed-TLS/mbedtls/pull/2054 added an unconditional #include "mbedtls/platform.h" in aes.c (and also platform_util.h is required pretty much everywhere).

Maybe the intent is to cause a build error (undefined references to mbedtls_xxx functions) in a configuration without MBEDTLS_PLATFORM_C, rather than silently compile something that wouldn't work? I don't see what errors we'd catch that way though: if the header is included, that still wouldn't provide the missing reference.

Problems with the current behavior

It's complex and we don't have documentation explaining what to do and why.

https://github.com/Mbed-TLS/mbedtls/issues/6118 is due to this complexity.

There's a historical rule to not include platform.h if MBEDTLS_PLATFORM_C, but we aren't actually following it everywhere (for example, it hasn't been followed in aes.c for years, since #2054). So whatever the rule is, we aren't actually following it.

I think the current behavior is outright wrong in some cases. For example, suppose I'm building an application with a small footprint for a freestanding environment, and I don't define MBEDTLS_PLATFORM_C (because I don't need anything from it) but I want provide my own calloc/free implementation. I should be able to use this configuration:

#undef MBEDTLS_PLATFORM_C
#define MBEDTLS_PLATFORM_MEMORY
#define MBEDTLS_PLATFORM_CALLOC_MACRO my_calloc
#define MBEDTLS_PLATFORM_FREE_MACRO my_free

This would make sense to me, but it doesn't work: check_config.h insists on enabling MBEDTLS_PLATFORM_C if MBEDTLS_PLATFORM_MEMORY is enabled. This has been the case since c0b6da3b439b4971aac241d15656abdcddc1616f, but I think it (and perhaps others like it) was included by mistake. The code in platform.c is needed only when the alternative calloc/free implementations are set via mbedtls_platform_set_calloc_free, not if they're set as macros (which is the common case on small-footprint builds). (We have a knowledge base article on malloc alternatives but unfortunately it doesn't mention the lightweight macro method, only the global variables+setter method.)

My proposal

Include mbedtls/platform.h unconditionally in all library code and sample programs that use any platform feature (mbedtls_calloc, `mbedtls_primtf, etc.). (That's probably almost if not all of them, so perhaps we can.)

~Since there is some risk of disruption (changing how some platform functions end up working in some “weird” configs), I don't think we should do this change in the LTS branch.~

We're going to do this even in 2.28 LTS because the risk of disruption is small and we really don't want people to take individual files. We should however have some kind of test to ensure that this actually doesn't change anything.

Goals of this issue

1. Document when mbedts/platform.h should be included.
2. Ensure that we follow the rule.
3. If needed, add configurations to the CI, or tweak existing ones, to ensure we keep following the rule in the future.

[daverodgman]

I think we should modify the philosophy document to remove the "loosely coupled" section. From a support point of view it's very difficult for us to support users who are taking individual pieces of the library.

[mpg]

Maybe the intent is that you can copy a few Mbed TLS source files into your application, and not bother copying platform.h if you haven't enabled MBEDTLS_PLATFORM_C? For example copy just aes.c and aes.h if you want AES and nothing else? While this is blessed in the Mbed TLS philosophy document, it mostly doesn't really work in practice. [...]

As far as I remember, that's indeed the reason. As you point out, that ship has sailed a few years ago, which was a conscious decision and IMO the right one, so as Dave says, we should update the philosophy document (which we should have done at the time we made the change).

[gilles-peskine-arm]

Based on discussions that happened here and in private:

• I've updated the philosophy document to say you can't take individual files.
• We won't try supporting that even in the 2.28 LTS, because it hasn't really been supported for years (it might or might not end up working depending on which files you take and which build options you enable), and also because it's a bad idea (makes it harder to apply security updates).
• So we're going to include platform.h liberally, and stop having ad hoc definitions for mbedtls_xxx aliases in individual .c files.

[gilles-peskine-arm]

Fixed by https://github.com/Mbed-TLS/mbedtls/pull/6291 (I forgot to include the magic text in the PR description).