heap_caps_check_integrity doesn’t find overflows (IDFGH-11050)

[MattiasTF]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v4.4.5-400-gc57d30e1df

Operating System used.

Linux

How did you build your project?

VS Code IDE

If you are using Windows, please specify command line type.

None

Development Kit.

Custom Board

Power Supply used.

USB

What is the expected behavior?

I expected the family of heap_caps_check_integrity functions to detect heap corruption, such as buffer overflows, when CONFIG_HEAP_POISONING_LIGHT=y is set.

What is the actual behavior?

Heap buffer overflows are not detected by any heap_caps_check_integrity function and only a free() will trigger a check that aborts the program.

Steps to reproduce.

My project contains the following code to deliberately cause a buffer overflow on the heap:

    uint32_t *ptr = static_cast<uint32_t *>(heap_caps_malloc(1 * sizeof(uint32_t), MALLOC_CAP_DEFAULT));
    printfln("array at %p", static_cast<void *>(ptr));

    printfln("canaries before corruption: 0x%08x 0x%08x", ptr[-2], ptr[1]);

    ptr[-2] = 0xdeaddead;
    ptr[ 0] = 0xff1234ff;
    ptr[ 1] = 0xdeaddead;

    printfln("canaries after corruption:  0x%08x 0x%08x", ptr[-2], ptr[1]);

    bool valid = heap_caps_check_integrity_all(true);
    printfln("heap_caps_check_integrity_all returned valid=%i", valid);

    heap_caps_free(ptr);

This is the output when run:

array at 0x3f80dca0
canaries before corruption: 0xabba1234 0xbaad5678
canaries after corruption:  0xdeaddead 0xdeaddead
heap_caps_check_integrity_all returned valid=1
CORRUPT HEAP: Bad head at 0x3f80dc98. Expected 0xabba1234 got 0xdeaddead

assert failed: multi_heap_free multi_heap_poisoning.c:259 (head != NULL)

• Allocate memory for one uint32_t.
• Deliberately over- and underflow the buffer.
• heap_caps_check_integrity_all claims that the heap is valid.
• heap_caps_free() detects the corruption.

Printing the memory before and after the buffer shows that the canaries are present and free() detects the corruption, which means that the heap is properly poisoned and the detection works, but heap_caps_check_integrity_all() does not.

Debug Logs.

No response

More Information.

multi_heap_poisoning.c contains multi_heap_internal_check_block_poisoning, which has code to check the poisoning of in-use blocks. In multi_heap.c, this function is used in the tlsf_check_hook. However, in heap_tslf.c, this hook is only called while checking the free lists and never for any in-use blocks. Therefore, corruption in in-use blocks is not detected.

The tlsf_check_hook can be added to the integrity_walker to check in-use blocks, similar to how it is called in tlsf_check. The check will fail, however, because the address of the block to check is incorrectly calculated in multi_heap_internal_check_block_poisoning. sizeof(poison_head_t) is added to the start address even though the start address is already positioned after the poison head. This was probably not found because the code never runs. If the start address is directly passed to tlsf_check_hook without the offset, the heap validation works as expected and both the buffer under- and overflow from the test code are detected.

My project is incompatible with IDF 5.x, so I cannot check if the issue is still present there, but looking at the relevant files on master shows that tlsf_check_hook is also called only during the free list verification and the calculation of the block address also contains the probably incorrect offset, so I expect both issues to be present there as well.

Here is a proof-of-concept patch that includes both changes mentioned above.

Edit: The proof-of-concept patch has been updated to print more useful information when a corruption was found.

[ESP-Marius]

Thanks for the thorough investigation!

I agree, heap_caps_check_integrity_all not finding certain overflows that free can detect does indeed seem counter-intuitive. We will take a look at it!

[MattiasTF]

Please note that the code from the proof-of-concept patch is incompatible with CONFIG_HEAP_TASK_TRACKING=y. Adding the removed sizeof(poison_head_t) doesn’t fix it, so I guess that there’s some other kind of offset that needs to be accounted for when heap task tracking is enabled.

[SoucheSouche]

@MattiasTF thanks for reporting this problem!

I updated the TLSF component to call thlsf_check_hook from tlsf_check_pool (via the integrity_walker) rather than from tlsf_check. Now the hook should be called on every block and not only on the free ones.

I will need to document the changes and add some testing. I will let yo know when the changes are merged internally.

Also note that it is currently not planed to patch the ROM implementation of the TLSF to incorporate this changes so the fixed version will only be available if CONFIG_HEAP_TLSF_USE_ROM_IMPL=n on the target that support the TLSF in ROM.