Remove local stack variable form MPU wrappers

It was possible for a third party that had already independently gained
the ability to execute injected code to achieve further privilege
escalation by branching directly inside a FreeRTOS MPU API wrapper
function with a manually crafted stack frame. This commit removes the
local stack variable `xRunningPrivileged` so that a manually crafted
stack frame cannot be used for privilege escalation by branching
directly inside a FreeRTOS MPU API wrapper.

We thank Certibit Consulting, LLC, Huazhong University of Science and
Technology and the SecLab team at Northeastern University for reporting
this issue.

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>

.github/lexicon.txt

@@ -2547,7 +2547,6 @@ vportgetheapstats
 vportinitialiseblocks
 vportisrstartfirststask
 vportraisebasepri
-vportresetprivilege
 vportsetmpuregistersetone
 vportsetuptimerinterrupt
 vportstartfirststask
@@ -2872,7 +2871,6 @@ xperiod
 xportgetcoreid
 xportgetfreeheapsize
 xportinstallinterrupthandler
-xportraiseprivilege
 xportregistercinterrupthandler
 xportregisterdump
 xportstartfirsttask

include/mpu_wrappers.h

@@ -173,36 +173,6 @@
         #define PRIVILEGED_DATA         __attribute__( ( section( "privileged_data" ) ) )
         #define FREERTOS_SYSTEM_CALL    __attribute__( ( section( "freertos_system_calls" ) ) )
 
-/**
- * @brief Calls the port specific code to raise the privilege.
- *
- * Sets xRunningPrivileged to pdFALSE if privilege was raised, else sets
- * it to pdTRUE.
- */
-        #define xPortRaisePrivilege( xRunningPrivileged )                  \
-    {                                                                      \
-        /* Check whether the processor is already privileged. */           \
-        ( xRunningPrivileged ) = portIS_PRIVILEGED();                      \
-                                                                           \
-        /* If the processor is not already privileged, raise privilege. */ \
-        if( ( xRunningPrivileged ) == pdFALSE )                            \
-        {                                                                  \
-            portRAISE_PRIVILEGE();                                         \
-        }                                                                  \
-    }
-
-/**
- * @brief If xRunningPrivileged is not pdTRUE, calls the port specific
- * code to reset the privilege, otherwise does nothing.
- */
-        #define vPortResetPrivilege( xRunningPrivileged ) \
-    {                                                     \
-        if( ( xRunningPrivileged ) == pdFALSE )           \
-        {                                                 \
-            portRESET_PRIVILEGE();                        \
-        }                                                 \
-    }
-
     #endif /* MPU_WRAPPERS_INCLUDED_FROM_API_FILE */
 
 #else /* portUSING_MPU_WRAPPERS */

portable/GCC/ARM_CM3_MPU/port.c

@@ -495,15 +495,26 @@ void vPortEndScheduler( void )
 void vPortEnterCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         portDISABLE_INTERRUPTS();
         uxCriticalNesting++;
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        portDISABLE_INTERRUPTS();
+        uxCriticalNesting++;
+    }
+#else
+    portDISABLE_INTERRUPTS();
+    uxCriticalNesting++;
 #endif
 }
 /*-----------------------------------------------------------*/
@@ -511,9 +522,10 @@ void vPortEnterCritical( void )
 void vPortExitCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         configASSERT( uxCriticalNesting );
         uxCriticalNesting--;
@@ -522,9 +534,29 @@ void vPortExitCritical( void )
         {
             portENABLE_INTERRUPTS();
         }
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        configASSERT( uxCriticalNesting );
+        uxCriticalNesting--;
+
+        if( uxCriticalNesting == 0 )
+        {
+            portENABLE_INTERRUPTS();
+        }
+    }
+#else
+    configASSERT( uxCriticalNesting );
+    uxCriticalNesting--;
+
+    if( uxCriticalNesting == 0 )
+    {
+        portENABLE_INTERRUPTS();
+    }
 #endif
 }
 /*-----------------------------------------------------------*/

portable/GCC/ARM_CM4_MPU/port.c

@@ -549,15 +549,26 @@ void vPortEndScheduler( void )
 void vPortEnterCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         portDISABLE_INTERRUPTS();
         uxCriticalNesting++;
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        portDISABLE_INTERRUPTS();
+        uxCriticalNesting++;
+    }
+#else
+    portDISABLE_INTERRUPTS();
+    uxCriticalNesting++;
 #endif
 }
 /*-----------------------------------------------------------*/
@@ -565,9 +576,10 @@ void vPortEnterCritical( void )
 void vPortExitCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         configASSERT( uxCriticalNesting );
         uxCriticalNesting--;
@@ -576,9 +588,29 @@ void vPortExitCritical( void )
         {
             portENABLE_INTERRUPTS();
         }
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        configASSERT( uxCriticalNesting );
+        uxCriticalNesting--;
+
+        if( uxCriticalNesting == 0 )
+        {
+            portENABLE_INTERRUPTS();
+        }
+    }
+#else
+    configASSERT( uxCriticalNesting );
+    uxCriticalNesting--;
+
+    if( uxCriticalNesting == 0 )
+    {
+        portENABLE_INTERRUPTS();
+    }
 #endif
 }
 /*-----------------------------------------------------------*/

portable/IAR/ARM_CM4F_MPU/port.c

@@ -464,13 +464,13 @@ void vPortEndScheduler( void )
 void vPortEnterCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         portDISABLE_INTERRUPTS();
         uxCriticalNesting++;
-
         /* This is not the interrupt safe version of the enter critical function so
          * assert() if it is being called from an interrupt context.  Only API
          * functions that end in "FromISR" can be used in an interrupt.  Only assert if
@@ -480,9 +480,37 @@ void vPortEnterCritical( void )
         {
             configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 );
         }
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        portDISABLE_INTERRUPTS();
+        uxCriticalNesting++;
+        /* This is not the interrupt safe version of the enter critical function so
+         * assert() if it is being called from an interrupt context.  Only API
+         * functions that end in "FromISR" can be used in an interrupt.  Only assert if
+         * the critical nesting count is 1 to protect against recursive calls if the
+         * assert function also uses a critical section. */
+        if( uxCriticalNesting == 1 )
+        {
+            configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 );
+        }
+    }
+#else
+    portDISABLE_INTERRUPTS();
+    uxCriticalNesting++;
+    /* This is not the interrupt safe version of the enter critical function so
+     * assert() if it is being called from an interrupt context.  Only API
+     * functions that end in "FromISR" can be used in an interrupt.  Only assert if
+     * the critical nesting count is 1 to protect against recursive calls if the
+     * assert function also uses a critical section. */
+    if( uxCriticalNesting == 1 )
+    {
+        configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 );
+    }
 #endif
 }
 /*-----------------------------------------------------------*/
@@ -490,21 +518,41 @@ void vPortEnterCritical( void )
 void vPortExitCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         configASSERT( uxCriticalNesting );
-
         uxCriticalNesting--;
 
         if( uxCriticalNesting == 0 )
         {
             portENABLE_INTERRUPTS();
         }
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        configASSERT( uxCriticalNesting );
+        uxCriticalNesting--;
+
+        if( uxCriticalNesting == 0 )
+        {
+            portENABLE_INTERRUPTS();
+        }
+    }
+#else
+    configASSERT( uxCriticalNesting );
+    uxCriticalNesting--;
+
+    if( uxCriticalNesting == 0 )
+    {
+        portENABLE_INTERRUPTS();
+    }
 #endif
 }
 /*-----------------------------------------------------------*/

portable/RVDS/ARM_CM4_MPU/port.c

@@ -551,15 +551,26 @@ void vPortEndScheduler( void )
 void vPortEnterCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         portDISABLE_INTERRUPTS();
         uxCriticalNesting++;
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        portDISABLE_INTERRUPTS();
+        uxCriticalNesting++;
+    }
+#else
+    portDISABLE_INTERRUPTS();
+    uxCriticalNesting++;
 #endif
 }
 /*-----------------------------------------------------------*/
@@ -567,9 +578,10 @@ void vPortEnterCritical( void )
 void vPortExitCritical( void )
 {
 #if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
-    BaseType_t xRunningPrivileged;
+    if( portIS_PRIVILEGED() == pdFALSE )
-    xPortRaisePrivilege( xRunningPrivileged );
+    {
-#endif
+        portRAISE_PRIVILEGE();
+        portMEMORY_BARRIER();
 
         configASSERT( uxCriticalNesting );
         uxCriticalNesting--;
@@ -578,9 +590,29 @@ void vPortExitCritical( void )
         {
             portENABLE_INTERRUPTS();
         }
+        portMEMORY_BARRIER();
 
-#if( configALLOW_UNPRIVILEGED_CRITICAL_SECTIONS == 1 )
+        portRESET_PRIVILEGE();
-    vPortResetPrivilege( xRunningPrivileged );
+        portMEMORY_BARRIER();
+    }
+    else
+    {
+        configASSERT( uxCriticalNesting );
+        uxCriticalNesting--;
+
+        if( uxCriticalNesting == 0 )
+        {
+            portENABLE_INTERRUPTS();
+        }
+    }
+#else
+    configASSERT( uxCriticalNesting );
+    uxCriticalNesting--;
+
+    if( uxCriticalNesting == 0 )
+    {
+        portENABLE_INTERRUPTS();
+    }
 #endif
 }
 /*-----------------------------------------------------------*/