Mask MPU region base addresses when writing to MPU_RBAR to fix potential kernel region setting override

[CocoDico78]

Description

While the docs are extensively mentioning that all regions should be aligned to power of two for correct MPU configuration, it is not explicit enough that not aligning (or not sanitizing) a user-defined region or a stack region may override MPU settings defined for higher priority kernel regions.

For example, for the ARM_CM3_MPU port, in port.c at lines 1318 (https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/ce221a8bb468e462ca6b435cef66a9636e00baf4/portable/GCC/ARM_CM3_MPU/port.c#L1318):

xMPUSettings->xRegion[ 0 ].ulRegionBaseAddress =
    ( ( uint32_t ) pxBottomOfStack ) |
    ( portMPU_REGION_VALID ) |
    ( portSTACK_REGION ); /* Region number. */

will escalate the actual region priority from 3 (value of portSTACK_REGION) to 7 (value of portPRIVILEGED_RAM_REGION) if pxBottomOfStack is maliciously or inadvertently shifted by 4 bytes (for example passing 0x20000004 instead of 0x20000000).

Same issue for user-defined regions which has the same bitwise calculation without masking.

This is in particular dangerous for implementers still using MPU wrappers v1 (which allows restricted tasks to create other restricted tasks, allowing to pass arbitrary misaligned stack pointers), and dangerous for implementers using MPU wrappers v2 who are not aware and might blindly trust user defined pointers thinking that the higher priority MPU regions will guard them.
Base addresses should be aligned to a power of two for proper MPU configuration. Masking ensures that a misaligned base address cannot modify less significant bits in the attribute reserved for other use, such as the region bits. Failing to mask the address may allow a malicious user to pass in misaligned addresses in a user-defined region or as stack buffer which could in turn override the settings for higher-priority kernel-defined regions.

In this pull-request, we enforce such masking on the ARM_CM3_MPU port as a proof-of-concept, but once approved this PR can be extended to apply similar change to all other MPU ports.

Test Steps

• Start a restricted task and pass a stack buffer starting at 0x20000004 in a user-defined region
• Observe that the previously set portPRIVILEGED_RAM_REGION settings are no longer overriden by the user-defined region

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

Follow up of a topic on the forum.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[aggarg]

@CocoDico78 I would have expected the change to clear the VALID and REGION bits in the RBAR register. Would something like the following work instead?

#define RBAR_ADDRESS_MASK 0xFFFFFFE0
xMPUSettings->xRegion[ 0 ].ulRegionBaseAddress =
            ( ( ( uint32_t ) __SRAM_segment_start__ ) & RBAR_ADDRESS_MASK ) | /* Base address. */
            ( portMPU_REGION_VALID ) |
            ( portSTACK_REGION );  

[CocoDico78]

@aggarg

Would something like the following work instead?

#define RBAR_ADDRESS_MASK 0xFFFFFFE0

Yes, this static mask would absolutely protect the bits [0-4] and thus solve the main issue here. This static mask would however still not prevent users from passing improperly aligned addresses: say the the user is configuring a region of size 64; bit 5 could still be incorrectly set if the user passes a base address not aligned with 64 but aligned with 32.

You are suggesting a simpler, static mask, while I'm presenting a solution with a dynamic mask that guarantees that the written ADDR is always correctly aligned. What are your thoughts? I'm fine either way.

EDIT: the simplicity argument compels me. First, let's solve the VALID and REGION masking. The discussion on ensuring alignment in the ADDR field is independent.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud