Kernel defense-in-depth hardening (heap_1, tasks, event_groups, stream_buffer, MSVC-MingW port)#1448
Open
AniruddhaKanhere wants to merge 5 commits into
Open
Conversation
… time Defect: pvPortMalloc() in heap_1.c can return a pointer outside the ucHeap array when configTOTAL_HEAP_SIZE is configured smaller than or equal to portBYTE_ALIGNMENT. Root cause: configADJUSTED_HEAP_SIZE is defined as ( configTOTAL_HEAP_SIZE - portBYTE_ALIGNMENT ). When configTOTAL_HEAP_SIZE is not larger than portBYTE_ALIGNMENT this unsigned subtraction underflows to a very large size_t value. The "enough room left" check in pvPortMalloc() compares an unsigned index against configADJUSTED_HEAP_SIZE, so the underflowed value defeats the check and an allocation can be handed out past the end of the heap array. Fix: add a compile-time (compiler, not preprocessor) size check so a nonsensical, too-small heap is rejected during the build. The compiler-level check still works when configTOTAL_HEAP_SIZE is defined with a cast such as ( ( size_t ) 0x2000 ). A host regression test kept outside this repository demonstrates the fault before the change and its absence afterwards (red then green).
AniruddhaKanhere
force-pushed
the
hardening/kernel-defense-in-depth
branch
from
685a3b0 to
2dc5967
Compare
Defect: xTaskCreate() / prvCreateTask() can under-allocate a task stack when a caller supplies a very large uxStackDepth, leading to out-of-bounds writes when the initial stack frame is set up. Root cause: the stack is allocated as ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ). If that product wraps size_t, the allocation is far smaller than requested, yet prvInitialiseNewTask() still computes the top of stack from the full uxStackDepth and writes the initial context past the end of the allocation. Fix: before allocating, reject creation when ( ( size_t ) uxStackDepth ) > ( SIZE_MAX / sizeof( StackType_t ) ), returning NULL (task not created) instead of proceeding with an under-sized buffer. A host regression test kept outside this repository demonstrates the fault before the change and its absence afterwards (red then green).
Defect: vPortGenerateSimulatedInterruptFromWindowsThread() in the MSVC-MingW simulator port performs a shift by a caller-supplied interrupt number without range checking it, giving undefined behavior for out-of-range values. Root cause: the function pends an interrupt via ( 1UL << ulInterruptNumber ) into ulPendingInterrupts, but does not verify ulInterruptNumber is within the width of that variable. A value greater than or equal to portMAX_INTERRUPTS makes the shift undefined. The task-context sibling vPortGenerateSimulatedInterrupt() already performs this bounds check. Fix: gate the operation on ( ulInterruptNumber < portMAX_INTERRUPTS ) in addition to the existing xPortRunning check, mirroring the task-context sibling so both entry points are consistent. A host regression test kept outside this repository demonstrates the fault before the change and its absence afterwards (red then green).
Defect: xStreamBufferSend() and xStreamBufferSendFromISR() on a message buffer can perform an out-of-bounds write when the required-space computation for a message overflows size_t and configASSERT() is compiled out. Root cause: for message buffers the code computes xRequiredSpace = xDataLengthBytes + sbBYTES_TO_STORE_MESSAGE_LENGTH and guards it only with configASSERT( xRequiredSpace > xDataLengthBytes ). When the addition wraps size_t, the wrapped (smaller) xRequiredSpace passes the subsequent space checks and the send proceeds to copy xDataLengthBytes bytes, writing past the buffer. With asserts disabled there is no guard at all. Fix: add a runtime check that returns 0 (nothing sent) when xRequiredSpace <= xDataLengthBytes, in both the task and FromISR send paths, so a wrapping message length is rejected regardless of the configASSERT() setting. A host regression test kept outside this repository demonstrates the fault before the change and its absence afterwards (red then green).
Defect: prvWriteMessageToBuffer() can write a truncated message-length header into a message buffer when the data length does not fit the configured configMESSAGE_BUFFER_LENGTH_TYPE and configASSERT() is compiled out, desynchronizing framing at the receiver. Root cause: the writer stores the length as a configMESSAGE_BUFFER_LENGTH_TYPE value xMessageLength and only guards the "fits without truncation" condition with configASSERT( ( size_t ) xMessageLength == xDataLengthBytes ). The subsequent write was gated solely on available space, so with asserts disabled a length that overflows the length type is written truncated while the full data is copied, corrupting the message framing seen by the receiver. Fix: gate the length-plus-data write on ( ( size_t ) xMessageLength == xDataLengthBytes ) in addition to the space check. When the length cannot be represented without truncation, take the "not enough space" path and write nothing. This runtime check is the only guard against truncation when asserts are disabled. A host regression test kept outside this repository demonstrates the fault before the change and its absence afterwards (red then green).
AniruddhaKanhere
force-pushed
the
hardening/kernel-defense-in-depth
branch
from
2dc5967 to
e2b6581
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A set of defense-in-depth hardening changes across the kernel and a few
portable layers. Each commit is a single, self-contained fix with a neutral
description of the defect, its root cause, and the guard added. No functional
behavior changes for correctly-configured, correctly-used callers; each change
only rejects or guards against a nonsensical or out-of-range input that would
otherwise lead to an under-allocation, a truncated result, or undefined
behavior.
One fix per commit (6 total):
heap_1: reject configTOTAL_HEAP_SIZE <= portBYTE_ALIGNMENT at compile time
Prevents an unsigned underflow of
configADJUSTED_HEAP_SIZEthat could letpvPortMalloc()hand out a pointer past the end of the heap array.tasks: reject stack depth whose byte size overflows size_t
Fails task creation instead of under-allocating when
uxStackDepth * sizeof( StackType_t )wrapssize_t.MSVC-MingW: bounds-check interrupt number in FromWindowsThread path
Adds the
< portMAX_INTERRUPTSguard the task-context sibling already has,avoiding an undefined shift in the simulator port.
event_groups: fail closed on FromISR bits above bit 31 on 64-bit tick builds
On 64-bit-tick builds the FromISR set/clear paths pass bits through a
uint32_tdaemon argument; bits above 31 were silently truncated. Nowrejected with
pdFALSE. 32-bit-tick builds are unchanged.stream_buffer: reject required-space overflow at runtime in message send
Promotes an existing
configASSERTto a runtime check so a wrappingmessage length is rejected even when asserts are compiled out.
stream_buffer: reject message length truncation at runtime in writer
Guards the length-header write so a length that cannot be represented by
configMESSAGE_BUFFER_LENGTH_TYPEdoes not desynchronize framing whenasserts are compiled out.
Testing
Each fix was developed and independently verified against a red-then-green host
regression test. The Kernel repository does not house the unit-test tree (CI
unit tests live in the FreeRTOS/FreeRTOS distribution repo under
test/unit-test), so these commits are code-only; corresponding tests can be
contributed to the distribution repo separately.