ARMmbed / ARMmbed/mbed-crypto

memory_buffer_alloc should use a sufficient default alignment

Open
#311 1 comment 0 reactions 0 assignees View on GitHub
Jira status: OPEN mirrored type: bug
Dominant language
C
Stars
108
Forks
95
PR merge metrics
No merged PRs in 30d

Description

### 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

Contributor guide

Open the contributing guide

Research direction

Development has moved to Mbed TLS, so first locate the corresponding memory buffer allocator there. Review include/mbedtls/memory_buffer_alloc.h and the scripts/config.pl reproduction, then inspect all.sh and component_test_memory_buffer_allocator; done means a suitable default alignment and UBSan coverage for the allocator.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.