espressif / esp-lwip

Fork of lwIP (https://savannah.nongnu.org/projects/lwip/) with ESP-IDF specific patches
Other
79 stars 126 forks source link

Wrongly identified mbedTLS memory configuration (IDFGH-10399) #57

Closed HamzaHajeir closed 8 months ago

HamzaHajeir commented 1 year ago

Hi there, I've had an issue that forced me to debug deeply last couple of days.

I'm using ALTCP_TLS_MBEDTLS, have failed to get a pcb out of calling altcp_tls_new(), that results in mbedtls fails to allocate memory even if very large memory was available, printing to the log:

IDF/components/mbedtls/mbedtls/library/ssl_tls.c:3857: alloc(16717 bytes) failed
mbedtls_ssl_setup failed

It starts by the user call [altcp_tls_create_config_server](https://github.com/lwip-tcpip/lwip/blob/e29870c15e8bf28eac9c811dd236c474f3f2008f/src/apps/altcp_tls/altcp_tls_mbedtls.c#LL863C33-L863C33), which creates tls config first, which in turn initializes memory by altcp_mbedtls_mem_init, which overrides mbedtls calloc/free.

Which its implementation compares against LWIP defined macro (MEM_SIZE), which is defaulted to 1600,

Overriding it is protected by the preprocessor ALTCP_MBEDTLS_PLATFORM_ALLOC but it holds a wrong dealing with mbedtls configuration.

MbedTLS has three states of memory configuration:

So what's proposed is to add a check to the preprocessor to become:

#if defined(MBEDTLS_PLATFORM_MEMORY) && \
     !defined(MBEDTLS_PLATFORM_FREE_MACRO) && \
    !defined(MBEDTLS_PLATFORM_STD_CALLOC)

Keep in mind that mbedTLS checks for misconfiguration (missing the correlated free/calloc or mixing std-defined with platform-defined): https://github.com/espressif/mbedtls/blob/15b55d406db3918bac88aaf5ef2c6e036d1e0f0e/include/mbedtls/check_config.h#L470-L496

david-cermak commented 8 months ago

I agree that the condition on enabling ALTCP_MBEDTLS_PLATFORM_ALLOC is probably wrong, but since you've already reported the problem upstream in https://savannah.nongnu.org/patch/index.php?10368 and we won't support altcp in IDF in the near future, I think we can close this issue.

We could possibly cherry-pick 601e1bb32663ad59faf809264e304850da88a3f3 to Espressif stable branches, but that's another issue and going to happen soon, once we switch to the latest lwip release.

There're several ways to work around this issue, you already mentioned redefining MEM_SIZE. Alternatively you can add your implementation to altcp_tls_mbedtls_mem.h and remove altcp_tls_mbedtls_mem.c from your sources.

Please reopen if needed.