TaskYield implementation for Cortex-M33 NTZ is incorrect

[alejoseb]

Describe the bug
TaskYield implementation is wrong for the Cortex-M33 NTZ port when using MPU. Currently, it only works if the restricted task is executed with privileged level. The reason is that TaskYield requires writing the control register that is only writable in the privileged mode:

#define portNVIC_INT_CTRL_REG ( *( ( volatile uint32_t * ) 0xe000ed04 ) )

void vPortYield( void ) /* PRIVILEGED_FUNCTION /
{
/ Set a PendSV to request a context switch. */
portNVIC_INT_CTRL_REG = portNVIC_PENDSVSET_BIT; // -> BUG this needs privileged level to be written

/* Barriers are normally not required but do ensure the code is
 * completely within the specified behaviour for the architecture. */
__asm volatile ( "dsb" ::: "memory" );
__asm volatile ( "isb" );

}

Target

• Development board: NUCLEO-L552
• Instruction Set Architecture: Cortex-M33
• IDE and version: STM32CubeIDE 1.10
• Toolchain and version: GCC 10.3

Host

• Host OS: Linux
• Version: Ubuntu 22.04

To Reproduce

Create a project with the port for Cortex-M33 NTZ with MPU enabled.
using the following configuration

#define configENABLE_MPU 1
#define configENABLE_TRUSTZONE 0

Create a restricted unprivileged task, and within the task call taskYIELD();

This will trigger a memfaullt exception.

Expected behavior
The task yields or forces a context switch

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context

The port for ARMv7 uses the SVC interface instead of the PendSV.
That could be a method to solve the issue.

Also, implementing taskYIELD(); as a syscall can solve the issue, since the execution
of this function needs privileged level to write to portNVIC_INT_CTRL_REG .

[Skptak]

Hey @alejoseb, thanks for filing this bug.
Want to make sure I'm following your bug report here.

You're saying that when using the ARM_CM33_NTZ/non_secure port that an unprivileged task would trigger a data abort when attempting to access the portNVIC_INT_CTRL_REG inside of the vPortYield() function?

void vPortYield( void ) /* PRIVILEGED_FUNCTION */
{
    /* Set a PendSV to request a context switch. */
    portNVIC_INT_CTRL_REG = portNVIC_PENDSVSET_BIT;

    /* Barriers are normally not required but do ensure the code is
     * completely within the specified behaviour for the architecture. */
    __asm volatile ( "dsb" ::: "memory" );
    __asm volatile ( "isb" );
}

Per Arm's documentation, it is true that attempting to access this register in unprivileged mode would lead to an abort.

However, the vPortYield() function is marked as a PRIVILEGED_FUNCTION:

/**
 * @brief Yield the processor.
 */
void vPortYield( void ) PRIVILEGED_FUNCTION;

/* Ensure API functions go in the privileged execution section. */
        #define PRIVILEGED_FUNCTION     __attribute__( ( section( "privileged_functions" ) ) )

When using an MPU port with unprivileged tasks these PRIVILEGED_FUNCTIONS should be placed into a separate section of flash. An example of this can be seen in the linker script, CORTEX_MPU_M33F_NXP_LPC55S69_MCUXpresso/Projects/MCUXpresso/NonSecure/FreeRTOSDemo_ns.ld:

    /* Privileged functions - Section needs to be 32 byte aligned to satisfy MPU requirements. */
    .privileged_functions : ALIGN(32)
    {
        . = ALIGN(32);
        __privileged_functions_start__ = .;
        *(privileged_functions)
        . = ALIGN(32);
        /* End address must be the last address in the region, therefore, -1. */
        __privileged_functions_end__ = . - 1;
    } > PRO

Then before starting the FreeRTOS Scheduler an MPU region is set that enforces protections on this flash section such that unprivileged tasks can not access it. This is done in prvSetupDefaultMPU():863

            /* Setup privileged flash as Read Only so that privileged tasks can
             * read it but not modify. */
            portMPU_RNR_REG = portPRIVILEGED_FLASH_REGION;
            portMPU_RBAR_REG = ( ( ( uint32_t ) __privileged_functions_start__ ) & portMPU_RBAR_ADDRESS_MASK ) |
                               ( portMPU_REGION_NON_SHAREABLE ) |
                               ( portMPU_REGION_PRIVILEGED_READ_ONLY );
            portMPU_RLAR_REG = ( ( ( uint32_t ) __privileged_functions_end__ ) & portMPU_RLAR_ADDRESS_MASK ) |
                               ( portMPU_RLAR_ATTR_INDEX0 ) |
                               ( portMPU_RLAR_REGION_ENABLE );

Where putting this all together, your unprivileged tasks shouldn't be hitting a memory abort when trying to access portNVIC_INT_CTRL_REG inside of vPortYield(). They should be hitting a memory abort when trying to run the vPortYield() function.

Your tasks should be calling functions such as vTaskDelay() or another blocking API(), such as waiting for a mutex or an event group, to inform the FreeRTOS-Scheduler that they should be swapped out for another task. Where the FreeRTOS-Kernel will then, while in a privileged operating mode, use this function to swap tasks.

Does that explain why the behaviour you are seeing isn't a bug, but actually intended?

[alejoseb]

Hi @Skptak,
The description of the error is correct. However, I already organized the code and memory sections as suggested following the official FreeRTOS documentation for the ARMv8M architecture.

Notice that vTaksDelay is implemented with mpu_wrappers, i.e., before executing the actual delay, the privilege will be elevated within the MPU_vTaskDelay wrapper which later calls the vTaksDelay on privileged level and then drops privileges after returning from the delay call.

In the case of vPortYield there is no MPU wrapper it is called directly, therefore the code within this function is executed in unprivileged mode regardless of being in PRIVILEGED_FUNCTIONS memory section. Allocating code in the PRIVILEGED_FUNCTIONS section does not elevate privileges.

Therefore this line:
portNVIC_INT_CTRL_REG = portNVIC_PENDSVSET_BIT;

will trigger an exception, since it is attempting to write a register ( portNVIC_INT_CTRL_REG ) that requires privileged level.

The register that is being accessed is detailed here: https://developer.arm.com/documentation/100235/0004/the-cortex-m33-peripherals/system-control-block/interrupt-control-and-state-register?lang=en

[Skptak]

Notice that vTaksDelay is implemented with mpu_wrappers, i.e., before executing the actual delay, the privilege will be elevated within the MPU_vTaskDelay wrapper which later calls the vTaksDelay on privileged level and then drops privileges after returning from the delay call.

That's actually exactly my point, an unprivileged FreeRTOS Task does not have the ability to directly call vTaskDelay(). It makes the "FreeRTOS System Call" to MPU_vTaskDelay():

void MPU_vTaskDelay( const TickType_t xTicksToDelay ) __attribute__ (( naked )) FREERTOS_SYSTEM_CALL;

As described in the FreeRTOS ARMv8M Architecture documentation you mentioned, Using FreeRTOS with Memory Protection Unit (MPU) Support, memory layout section.

In the case of vPortYield there is no MPU wrapper it is called directly, therefore the code within this function is executed in unprivileged mode regardless of being in PRIVILEGED_FUNCTIONS memory section. Allocating code in the PRIVILEGED_FUNCTIONS section does not elevate privileges.

Therefore this line: portNVIC_INT_CTRL_REG = portNVIC_PENDSVSET_BIT;

will trigger an exception, since it is attempting to write a register ( portNVIC_INT_CTRL_REG ) that requires privileged level.

This same wrapping is essentially what is happening with vPortYield(). In a FreeRTOS system that is using an MPU an unprivileged task is not allowed to directly call vPortYield(). Tasks should instead be performing some other operation that leads to the FreeRTOS-Kernel calling portYIELD(), such as xQueueGenericSend(), which as part of its execution path may call portYield().

In the Armv7-M ports you mentioned in your original post this function is guarded inside of the FreeRTOS_SVC_Handler itself:

        case portSVC_YIELD:
            portNVIC_INT_CTRL_REG = portNVIC_PENDSVSET_BIT;

            /* Barriers are normally not required
             * but do ensure the code is completely
             * within the specified behaviour for the
             * architecture. */
            __asm volatile ( "dsb" ::: "memory" );
            __asm volatile ( "isb" );

            break;

            #if ( configENFORCE_SYSTEM_CALLS_FROM_KERNEL_ONLY == 1 )
                case portSVC_RAISE_PRIVILEGE: /* Only raise the privilege, if the
                                               * svc was raised from any of the
                                               * system calls. */

                    if( ( ulPC >= ( uint32_t ) __syscalls_flash_start__ ) &&
                        ( ulPC <= ( uint32_t ) __syscalls_flash_end__ ) )
                    {
                        __asm volatile
                        (
                            "   mrs r1, control     \n" /* Obtain current control value. */
                            "   bic r1, #1          \n" /* Set privilege bit. */
                            "   msr control, r1     \n" /* Write back new control value. */
                            ::: "r1", "memory"
                        );
                    }

Where if compiling the port WITHOUT configENFORE_SYSTEM_CALLS_FROM_KERNEL_ONLY set to 1 we generate a compiler warning say we do not recommend that

#ifndef configENFORCE_SYSTEM_CALLS_FROM_KERNEL_ONLY
    #warning "configENFORCE_SYSTEM_CALLS_FROM_KERNEL_ONLY is not defined. We recommend defining it to 1 in FreeRTOSConfig.h for better security. www.FreeRTOS.org/FreeRTOS-V10.3.x.html"
    #define configENFORCE_SYSTEM_CALLS_FROM_KERNEL_ONLY    0
#endif

[alejoseb]

What is the reasoning for preventing calling vPortYield() from an unprivileged task?

What are the security implications?

Yielding the execution of a task is a basic primitive of any RTOS. It simply lets know the kernel that the task has nothing to do at that moment and other tasks that are ready can be executed. Calling it only as part of another function doesn't make sense.

Also, the idea is not to disable configENFORE_SYSTEM_CALLS_FROM_KERNEL_ONLY. The idea is to have a proper syscall for ARMv7 and Armv8. If disabling that control is needed, then that implies that ARMv7 is also missing a proper syscall.

Also, If portyield should not be accessible from user space code then that function should be static. Currently it is not static and is accessible from user space.

A work around could be to call the delay function with 0 as a parameter. However this will reduce portability needing different source code when executing in privileged and unprivileged mode.

This is a relevant discussion about this topic: https://www.freertos.org/FreeRTOS_Support_Forum_Archive/March_2011/freertos_vTaskDelay_0_vs_taskYIELD_4401035.html

[kar-rahul-aws]

Hi @alejoseb
Thank you for bringing this to our attention. We will fix it.

[ydhuang28]

@alejoseb In my opinion, this is not a bug. The whole reason we have MPU regions and memory protection is because it provides the ability for the application programmer to choose whether they want protection for their tasks or not. If you'd like to have MPU protection for your task, simply make it privileged. Privileged tasks are not meant to be reserved for the FreeRTOS Kernel. Otherwise, we are not preventing you from calling yield; we are simply providing a wrapper so that your yield call (or, in this case, MPU_vTaskDelay( 0 ) call) can be done safely.

[ydhuang28]

@alejoseb If you'd like to discuss more, please feel free to open a question on the FreeRTOS.org forum. (If you've already done so, I do apologize for the duplicate instructions.)

[alejoseb]

@ydhuang28 Sorry, but this statement is wrong
"If you'd like to have MPU protection for your task, simply make it privileged."

If you make a task privileged then this task is not protected at all. In fact, a privileged task can access any memory region (flash, ram, peripherals) but the kernel.

So, if you have a bug or vulnerability in your privileged task, then that will not be contained and potentially affect other memory areas.
Ideally, we want to run all user space code in the unprivileged mode, then any bug or memory overflow will be contained. This is the basic principle of compartmentalization and privilege segregation. I have reported previously security bugs to AWS regarding this point, you can check your own security advisory.

This issue is not security related so I didn't report directly to AWS.

I understand that you can call the delay function with (0) as a parameter and avoid having a proper syscall for the taskyield() function. As I mentioned earlier this reduces code portability, since you will need different code bases to run privileged and unprivileged.

There is no reason to continue the discussion in the forum, but it is better to clarify the right usage of the MPU, the privileged and unprivileged mode.

[aggarg]

@alejoseb This issue is fixed in this PR - #817.

Thank you for reporting this.

[alejoseb]

@aggarg thanks the PR solves the issue.