ARMmbed / ARMmbed/mbed-crypto

Possible race in mbedtls_memory_buffer_alloc_status()

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

- Target: STM32L476RG (ARM Cortex-M4F)
- Toolchain: gcc-arm-none-eabi 8.2.1
- Tools: mbed-cli 1.10.1
- mbed-os version: 5.12.1
- mbedtls version: 2.7.6 (is however still present in newest version)

In a firmware of mine, I have observed a crash (HardFault) when printing the status of the memory buffer using the function `mbedtls_memory_buffer_alloc_status()`. The crash log is as follows:

```
[02][01][02][T][2019-10-16 10:30:08 / 1571221808152] Memory at thread end
Current use: 102 blocks / 12544 bytes, max: 126 blocks / 20916 bytes (total 24948 bytes), alloc / free: 3277775 / 3277687, total buffer len 28672

Block list
HDR: PTR( 536917680), PREV( 0), NEXT( 536917928), ALLOC(1), SIZE( 216)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536917928), PREV( 536917680), NEXT( 536918216), ALLOC(1), SIZE( 256)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536918216), PREV( 536917928), NEXT( 536918668), ALLOC(1), SIZE( 420)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536918668), PREV( 536918216), NEXT( 536918872), ALLOC(1), SIZE( 172)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536918872), PREV( 536918668), NEXT( 536918952), ALLOC(1), SIZE( 48)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536918952), PREV( 536918872), NEXT( 536919032), ALLOC(1), SIZE( 48)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536919032), PREV( 536918952), NEXT( 536919068), ALLOC(1), SIZE( 4)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536919068), PREV( 536919032), NEXT( 536919408), ALLOC(1), SIZE( 308)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536919408), PREV( 536919068), NEXT( 536919852), ALLOC(1), SIZE( 412)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536919852), PREV( 536919408), NEXT( 536920056), ALLOC(1), SIZE( 172)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920056), PREV( 536919852), NEXT( 536920120), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920120), PREV( 536920056), NEXT( 536920184), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920184), PREV( 536920120), NEXT( 536920220), ALLOC(1), SIZE( 4)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920220), PREV( 536920184), NEXT( 536920708), ALLOC(1), SIZE( 456)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920708), PREV( 536920220), NEXT( 536920772), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920772), PREV( 536920708), NEXT( 536920976), ALLOC(1), SIZE( 172)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536920976), PREV( 536920772), NEXT( 536921040), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921040), PREV( 536920976), NEXT( 536921104), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921104), PREV( 536921040), NEXT( 536921140), ALLOC(1), SIZE( 4)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921140), PREV( 536921104), NEXT( 536921344), ALLOC(1), SIZE( 172)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921344), PREV( 536921140), NEXT( 536921408), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921408), PREV( 536921344), NEXT( 536921472), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921472), PREV( 536921408), NEXT( 536921536), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921536), PREV( 536921472), NEXT( 536921572), ALLOC(1), SIZE( 4)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536921572), PREV( 536921536), NEXT( 536925248), ALLOC(0), SIZE( 3644)
FPREV( 536937688), FNEXT( 536936896)
HDR: PTR( 536925248), PREV( 536921572), NEXT( 536925588), ALLOC(1), SIZE( 308)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536925588), PREV( 536925248), NEXT( 536925668), ALLOC(1), SIZE( 48)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536925668), PREV( 536925588), NEXT( 536925748), ALLOC(1), SIZE( 48)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536925748), PREV( 536925668), NEXT( 536925860), ALLOC(1), SIZE( 80)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536925860), PREV( 536925748), NEXT( 536925960), ALLOC(1), SIZE( 68)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536925960), PREV( 536925860), NEXT( 536926064), ALLOC(1), SIZE( 72)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536926064), PREV( 536925960), NEXT( 536926516), ALLOC(1), SIZE( 420)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536926516), PREV( 536926064), NEXT( 536927024), ALLOC(0), SIZE( 476)
FPREV( 536932152), FNEXT( 536929784)
HDR: PTR( 536927024), PREV( 536926516), NEXT( 536927468), ALLOC(1), SIZE( 412)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536927468), PREV( 536927024), NEXT( 536927808), ALLOC(1), SIZE( 308)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536927808), PREV( 536927468), NEXT( 536927872), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536927872), PREV( 536927808), NEXT( 536927908), ALLOC(1), SIZE( 4)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536927908), PREV( 536927872), NEXT( 536927976), ALLOC(1), SIZE( 36)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536927976), PREV( 536927908), NEXT( 536928040), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536928040), PREV( 536927976), NEXT( 536928104), ALLOC(1), SIZE( 32)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536928104), PREV( 536928040), NEXT( 536928140), ALLOC(1), SIZE( 4)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536928140), PREV( 536928104), NEXT( 536928240), ALLOC(1), SIZE( 68)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536928240), PREV( 536928140), NEXT( 536928280), ALLOC(1), SIZE( 8)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536928280), PREV( 536928240), NEXT( 536928324), ALLOC(1), SIZE( 12)
FPREV( 0), FNEXT( 0)
HDR: PTR( 536928324), PREV( 536928280), NEXT( 536928400), ALLOC(0), SIZE( 44)
FPREV( 1), FNEXT( 536928240)
HDR: PTR( 68), PREV( 134381637), NEXT( 134381637), ALLOC(134381637), SIZE( 134381637)
FPREV( 134381637), FNEXT( 134381637)
HDR: PTR( 134381637), PREV( 602932256), NEXT(4143448061), ALLOC(267913456), SIZE( 2098101)
FPREV( 136315135), FNEXT( 117440701)

++ MbedOS Fault Handler ++

FaultType: HardFault

Context:
R0 : 20000844
R1 : 34000000
R2 : 00000000
R3 : 20000488
R4 : F6F7FFFD
R5 : 20000484
R6 : 00320377
R7 : 00007000
R8 : 20015CC0
R9 : 200058D4
R10 : 2000595C
R11 : 200058D8
R12 : 00000000
SP : 100025A8
LR : 0800DCAB
PC : 0800DC5C
xPSR : 01010000
PSP : 10002540
MSP : 10007FC0
CPUID: 410FC241
HFSR : 40000000
MMFSR: 00000000
BFSR : 00000082
UFSR : 00000000
DFSR : 00000008
AFSR : 00000000
BFAR : F6F8000D
Mode : Thread
Priv : Privileged
Stack: PSP

-- MbedOS Fault Handler --

++ MbedOS Error Info ++
Error Status: 0x0 Code: 0 Module: 0
Error Message: Fault exception
Location: 0x0
Error Value: 0x0
Current Thread: � � Id: 0x0 Entry: 0x0 StackSize: 0x0 StackMem: 0x0 SP: 0x0
For more info, visit: https://mbed.com/s/error?error=0x00000000&tgt=DEV
-- MbedOS Error Info --
```

The firmware is fairly complex and features multiple threads doing handshakes or encrypting stuff. So it can definitely happen that there concurrent calls to the buffer allocation functions. However, I have also implemented the `MBEDTLS_THREADING_ALT` functions using the mbed-os provided `Mutex` class and called `mbedtls_threading_set_alt()` accordingly. This mutex layer works, I have tested it independently.

The crash takes place (by looking at the `PC`) at the last line

```cpp
mbedtls_fprintf( stderr, "HDR: PTR(%10u), PREV(%10u), NEXT(%10u), "
"ALLOC(%u), SIZE(%10u)\n",
(size_t) hdr, (size_t) hdr->prev, (size_t) hdr->next,
```
Where it does a dereference of the `hdr` pointer.

What you can see from the log is while printing the free list of the heap, it suddenly jumps to pointers it shouldn't. Note how there's there's a break from `PTR( 536928324)` to `PTR( 68)` (with an allocation size which exceeds the amount of memory the device has).

```
HDR: PTR( 536928324), PREV( 536928280), NEXT( 536928400), ALLOC(0), SIZE( 44)
FPREV( 1), FNEXT( 536928240)
HDR: PTR( 68), PREV( 134381637), NEXT( 134381637), ALLOC(134381637), SIZE( 134381637)
FPREV( 134381637), FNEXT( 134381637)
HDR: PTR( 134381637), PREV( 602932256), NEXT(4143448061), ALLOC(267913456), SIZE( 2098101)
FPREV( 136315135), FNEXT( 117440701)
```

When looking at the code it seems to me that this is a race which occurs when calling `mbedtls_memory_buffer_alloc_status()`. All the allocation function lock the `heap.mutex` while modifying the heap (see e.g. `buffer_alloc_calloc_mutexed()`). however, the buffer alloc status function does not. It just iterates through the linked list and follows every pointer. This behavior is still in there in newest code.

https://github.com/ARMmbed/mbed-crypto/blob/150d5777802834f0b7084fad95b32ebbc1e75d16/library/memory_buffer_alloc.c#L509-L530

Thus I think my observed crash was caused by a race in which a `free()` operation took place while printing the buffer, and thus it printed a correct `NEXT` pointer but when assigning `cur = cur->next_free;` the value was already modified to some other value.

I thus propse to add the following code at the beginning and end of the function:

```cpp
#if defined(MBEDTLS_THREADING_C)
if (mbedtls_mutex_lock(&heap.mutex) != 0) {
return;
}
#endif

[..]
#if defined(MBEDTLS_THREADING_C)
mbedtls_mutex_unlock(&heap.mutex);
#endif
```

I have tried to reproduce this exact crash but haven't succeeded. Triggering / loosing this race seems to be quite hard, but as my log file shows, possible.

### Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

Contributor guide

Open the contributing guide

Research direction

Start in library/memory_buffer_alloc.c at mbedtls_memory_buffer_alloc_status(), then compare its traversal with the locking used by buffer_alloc_calloc_mutexed() and other allocation functions. Confirm the status walk cannot race with heap modifications, and verify that concurrent allocation and free activity no longer produces invalid pointers or a HardFault.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 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.