From 9c21165c1453230b9bde8be54ba534346a79be3a Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Wed, 29 Jan 2025 08:35:51 -0800
Subject: [PATCH] [key-manager] clarify `otPlatRadioSetMacKey()` and add
 counter safeguard (#11201)

This commit clarifies the intended behavior of the radio platform API
`otPlatRadioSetMacKey()` by recommending that implementations also
clear the MAC frame counter value (tracked by the radio).

It also adds a safeguard for this in `KeyManager` by changing the
order of operations. The MAC frame counter is now reset before
updating the key sequence and MAC keys. A comment explains the
rationale behind this ordering. This is to avoid potential issue
where a large counter value may be used with the new MAC key which
could then hinder frame transmission for a long duration.
---
 include/openthread/instance.h       |  2 +-
 include/openthread/platform/radio.h |  4 ++++
 src/core/thread/key_manager.cpp     | 29 +++++++++++++++++++++++++++--
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/include/openthread/instance.h b/include/openthread/instance.h
index 8157fea28..700299dd1 100644
--- a/include/openthread/instance.h
+++ b/include/openthread/instance.h
@@ -52,7 +52,7 @@ extern "C" {
  *
  * @note This number versions both OpenThread platform and user APIs.
  */
-#define OPENTHREAD_API_VERSION (473)
+#define OPENTHREAD_API_VERSION (474)
 
 /**
  * @addtogroup api-instance
diff --git a/include/openthread/platform/radio.h b/include/openthread/platform/radio.h
index 5309c2f2b..8f7f38f9a 100644
--- a/include/openthread/platform/radio.h
+++ b/include/openthread/platform/radio.h
@@ -702,6 +702,10 @@ void otPlatRadioSetRxOnWhenIdle(otInstance *aInstance, bool aEnable);
  *
  * Is used when radio provides OT_RADIO_CAPS_TRANSMIT_SEC capability.
  *
+ * The radio platform should reset the current security MAC frame counter tracked by the radio on this call. While this
+ * is highly recommended, the OpenThread stack, as a safeguard, will also reset the frame counter using the
+ * `otPlatRadioSetMacFrameCounter()` before calling this API.
+ *
  * @param[in]   aInstance    A pointer to an OpenThread instance.
  * @param[in]   aKeyIdMode   The key ID mode.
  * @param[in]   aKeyId       Current MAC key index.
diff --git a/src/core/thread/key_manager.cpp b/src/core/thread/key_manager.cpp
index 6596bf75e..a9ab0b0ff 100644
--- a/src/core/thread/key_manager.cpp
+++ b/src/core/thread/key_manager.cpp
@@ -371,12 +371,37 @@ void KeyManager::SetCurrentKeySequence(uint32_t aKeySequence, KeySeqUpdateFlags
         VerifyOrExit(mKeySwitchGuardTimer == 0);
     }
 
-    mKeySequence = aKeySequence;
-    UpdateKeyMaterial();
+    // MAC frame counters are reset before updating keys. This order
+    // safeguards against issues that can arise when the radio
+    // platform handles TX security and counter assignment.  The
+    // radio platform might prepare an enhanced ACK to a received
+    // frame from an parallel (e.g., ISR) context, which consumes
+    // a MAC frame counter value.
+    //
+    // Ideally, a call to `otPlatRadioSetMacKey()`, which sets the MAC
+    // keys on the radio, should also reset the frame counter tracked
+    // by the radio. However, if this is not implemented by the radio
+    // platform, resetting the counter first ensures new keys always
+    // start with a zero counter and avoids potential issue below.
+    //
+    // If the MAC key is updated before the frame counter is cleared,
+    // the radio could receive and send an enhanced ACK between these
+    // two actions, possibly using the new MAC key with a larger
+    // (current) frame counter value. This could then prevent the
+    // receiver from accepting subsequent transmissions after the
+    // frame counter reset for a long time.
+    //
+    // While resetting counters first might briefly cause an enhanced
+    // ACK to be sent with the old key and a zero counter (which might
+    // be rejected by the receiver), this is a transient issue that
+    // quickly resolves itself.
 
     SetAllMacFrameCounters(0, /* aSetIfLarger */ false);
     mMleFrameCounter = 0;
 
+    mKeySequence = aKeySequence;
+    UpdateKeyMaterial();
+
     ResetKeyRotationTimer();
 
     if (aFlags & kResetGuardTimer)