memory_buffer_alloc should use a sufficient default alignment #311

New issue

Open

opened 2019-10-31 10:29:44 -05:00 by gilles-peskine-arm · 1 comment

gilles-peskine-arm commented

2019-10-31 10:29:44 -05:00

(Migrated from github.com)

Description

The MEMORY_BUFFER_ALLOC module defaults to aligning blocks on a 4-byte boundary.

include/mbedtls/memory_buffer_alloc.h:#define MBEDTLS_MEMORY_ALIGN_MULTIPLE       4 /**< Align on multiples of this value */

On architectures that require 8- or 16-byte alignment for some data, this results in buggy code.

The fix is to default MBEDTLS_MEMORY_ALIGN_MULTIPLE to something sensible. In C11, that's _Alignof(max_align_t). In older C dialects, should we use sizeof(char*) (backward compatible) or 2 * sizeof(char*) (I don't know of a machine where it's required, but it can improve performance with vector instructions)?

To test, we should run a job in all.sh with MBEDTLS_MEMORY_BUFFER_ALLOC_C enabled and UBSan enabled (it detects misaligned pointer accesses). At the moment, MBEDTLS_MEMORY_BUFFER_ALLOC_C is tested via config.pl full. In Mbed TLS, we've changed that to a dedicated component_test_memory_buffer_allocator. We should wait until this change is sideported to crypto, then modify the test component to enable UBSan.

To reproduce:

scripts/config.pl set MBEDTLS_MEMORY_BUFFER_ALLOC_C
scripts/config.pl set MBEDTLS_MEMORY_DEBUG
scripts/config.pl set MBEDTLS_PLATFORM_MEMORY
make CFLAGS='-fsanitize=undefined' LDFLAGS='-fsanitize=undefined' lib tests
make test

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug

### Description The MEMORY_BUFFER_ALLOC module defaults to aligning blocks on a 4-byte boundary. ``` include/mbedtls/memory_buffer_alloc.h:#define MBEDTLS_MEMORY_ALIGN_MULTIPLE 4 /**< Align on multiples of this value */ ``` On architectures that require 8- or 16-byte alignment for some data, this results in buggy code. The fix is to default `MBEDTLS_MEMORY_ALIGN_MULTIPLE` to something sensible. In C11, that's `_Alignof(max_align_t)`. In older C dialects, should we use `sizeof(char*)` (backward compatible) or `2 * sizeof(char*)` (I don't know of a machine where it's required, but it can improve performance with vector instructions)? To test, we should run a job in `all.sh` with `MBEDTLS_MEMORY_BUFFER_ALLOC_C` enabled and UBSan enabled (it detects misaligned pointer accesses). At the moment, `MBEDTLS_MEMORY_BUFFER_ALLOC_C` is tested via `config.pl` full. In Mbed TLS, we've changed that to a dedicated `component_test_memory_buffer_allocator`. We should wait until this change is sideported to crypto, then modify the test component to enable UBSan. To reproduce: ``` scripts/config.pl set MBEDTLS_MEMORY_BUFFER_ALLOC_C scripts/config.pl set MBEDTLS_MEMORY_DEBUG scripts/config.pl set MBEDTLS_PLATFORM_MEMORY make CFLAGS='-fsanitize=undefined' LDFLAGS='-fsanitize=undefined' lib tests make test ``` ### Issue request type [ ] Question [ ] Enhancement [x] Bug

ciarmcom commented

2019-10-31 16:47:08 -05:00

(Migrated from github.com)