Possible race in mbedtls_memory_buffer_alloc_status()

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

	void mbedtls_memory_buffer_alloc_status( void )
	{
	mbedtls_fprintf( stderr,
	"Current use: %zu blocks / %zu bytes, max: %zu blocks / "
	"%zu bytes (total %zu bytes), alloc / free: %zu / %zu\n",
	heap.header_count, heap.total_used,
	heap.maximum_header_count, heap.maximum_used,
	heap.maximum_header_count * sizeof( memory_header )
	+ heap.maximum_used,
	heap.alloc_count, heap.free_count );

	if( heap.first->next == NULL )
	{
	mbedtls_fprintf( stderr, "All memory de-allocated in stack buffer\n" );
	}
	else
	{
	mbedtls_fprintf( stderr, "Memory currently allocated:\n" );
	debug_chain();
	}
	}

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Possible race in mbedtls_memory_buffer_alloc_status() #302

Description

Issue request type

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Possible race in mbedtls_memory_buffer_alloc_status() #302

Description

Description

Issue request type

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions