From e14bee52095dac5a41f770604f3fabb8dbaaadf8 Mon Sep 17 00:00:00 2001
From: Steven Cartmell <steve.cartmell@arm.com>
Date: Tue, 19 Dec 2017 13:35:43 +0000
Subject: [PATCH] Fix potential race condition in critical section HAL API

Call underlying HAL implementation to enter critical section/disable interrupts
before incrementing the global critical section counter.

Modify HAL implementations to track first entrances to the critical section and
only update the saved state on first enter.
---
 .../hal_patch/critical_section_api.c            | 17 +++++++++++++++--
 hal/mbed_critical_section_api.c                 | 13 +++++++++++--
 platform/mbed_critical.c                        |  8 +++-----
 .../TARGET_NRF5/critical_section_api.c          | 14 +++++++++++++-
 4 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_MCU_NRF51822/hal_patch/critical_section_api.c b/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_MCU_NRF51822/hal_patch/critical_section_api.c
index 237b14817c..d3d2a9afa9 100644
--- a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_MCU_NRF51822/hal_patch/critical_section_api.c
+++ b/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_MCU_NRF51822/hal_patch/critical_section_api.c
@@ -28,11 +28,13 @@ static union {
 } _state = {0};
 
 static bool _use_softdevice_routine = false;
+static bool _state_saved = false;
 
 void hal_critical_section_enter(void)
 {
     // Fetch the current state of interrupts
     uint32_t primask = __get_PRIMASK();
+    uint8_t temp_state = 0;
 
     // If interrupts are enabled, try to use the soft device
     uint8_t sd_enabled;
@@ -40,8 +42,12 @@ void hal_critical_section_enter(void)
         (sd_softdevice_is_enabled(&sd_enabled) == NRF_SUCCESS) &&
         (sd_enabled == 1)) {
         // If the softdevice can be used, use it.
-        sd_nvic_critical_region_enter(&_state._sd_state);
+        sd_nvic_critical_region_enter(&temp_state);
         _use_softdevice_routine = true;
+
+        if (_state_saved == false) {
+            _state._sd_state = temp_state;
+        }
     } else {
         // If interrupts are enabled, disable them.
         if (primask == 0) {
@@ -50,13 +56,20 @@ void hal_critical_section_enter(void)
 
         // Store PRIMASK state, it will be restored when exiting critical
         // section.
-        _state._PRIMASK_state = primask;
         _use_softdevice_routine = false;
+
+        if (_state_saved == false) {
+            _state._PRIMASK_state = primask;
+        }
     }
+
+    _state_saved = true;
 }
 
 void hal_critical_section_exit(void)
 {
+    _state_saved = false;
+
     // Restore the state as it was prior to entering the critical section.
     if (_use_softdevice_routine) {
         sd_nvic_critical_region_exit(_state._sd_state)
diff --git a/hal/mbed_critical_section_api.c b/hal/mbed_critical_section_api.c
index fb89fadf3a..39f2398622 100644
--- a/hal/mbed_critical_section_api.c
+++ b/hal/mbed_critical_section_api.c
@@ -21,6 +21,7 @@
 #include <stdbool.h>
 
 static volatile bool critical_interrupts_enabled = false;
+static volatile bool state_saved = false;
 
 static bool are_interrupts_enabled(void)
 {
@@ -34,17 +35,25 @@ static bool are_interrupts_enabled(void)
 
 MBED_WEAK void hal_critical_section_enter(void)
 {
-    critical_interrupts_enabled = are_interrupts_enabled();
+    const bool interrupt_state = are_interrupts_enabled();
 
     __disable_irq();
-}
 
+    if (state_saved == true) {
+        return;
+    }
+
+    critical_interrupts_enabled = interrupt_state;
+    state_saved = true;
+}
 
 MBED_WEAK void hal_critical_section_exit()
 {
     // Interrupts must be disabled on invoking an exit from a critical section
     MBED_ASSERT(!are_interrupts_enabled());
 
+    state_saved = false;
+
     // Restore the IRQs to their state prior to entering the critical section
     if (critical_interrupts_enabled == true) {
         __enable_irq();
diff --git a/platform/mbed_critical.c b/platform/mbed_critical.c
index 39f2adcebf..fcdcca60b7 100644
--- a/platform/mbed_critical.c
+++ b/platform/mbed_critical.c
@@ -66,11 +66,9 @@ void core_util_critical_section_enter(void)
     MBED_ASSERT(critical_section_reentrancy_counter < UINT32_MAX);
 #endif /* FEATURE_UVISOR */
 
-    if (critical_section_reentrancy_counter == 0) {
-        hal_critical_section_enter();
-    }
+    hal_critical_section_enter();
 
-    critical_section_reentrancy_counter++;
+    ++critical_section_reentrancy_counter;
 }
 
 void core_util_critical_section_exit(void)
@@ -85,7 +83,7 @@ void core_util_critical_section_exit(void)
         return;
     }
 
-    critical_section_reentrancy_counter--;
+    --critical_section_reentrancy_counter;
 
     if (critical_section_reentrancy_counter == 0) {
         hal_critical_section_exit();
diff --git a/targets/TARGET_NORDIC/TARGET_NRF5/critical_section_api.c b/targets/TARGET_NORDIC/TARGET_NRF5/critical_section_api.c
index b560f13b6b..a329d18941 100644
--- a/targets/TARGET_NORDIC/TARGET_NRF5/critical_section_api.c
+++ b/targets/TARGET_NORDIC/TARGET_NRF5/critical_section_api.c
@@ -16,16 +16,28 @@
  */
 #include "nrf_nvic.h"
 
+#include <stdbool.h>
 #include <stdint.h> // uint32_t, UINT32_MAX
 
 static uint8_t _sd_state = 0;
+static volatile bool _state_saved = false;
 
 void hal_critical_section_enter(void)
 {
-    sd_nvic_critical_region_enter(&_sd_state);
+    uint8_t temp_state = 0;
+    sd_nvic_critical_region_enter(&temp_state);
+
+    if (_state_saved == true) {
+        return;
+    }
+
+    _sd_state = temp_state;
+    _state_saved = true;
 }
 
 void hal_critical_section_exit(void)
 {
+    _state_saved = false;
+
     sd_nvic_critical_region_exit(_sd_state);
 }