[Silabs]Fix a memory leak in Efr32PsaOperationalKeystore (#30292)

* Free mPendingKeypair before setting it to null * add clarifications. rename function
project-chip · Nov 29, 2023 · fdb3cab · fdb3cab
1 parent 3687e6b
commit fdb3cab
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 14 deletions.
diff --git a/src/platform/silabs/efr32/Efr32OpaqueKeypair.h b/src/platform/silabs/efr32/Efr32OpaqueKeypair.h
@@ -132,7 +132,7 @@ class EFR32OpaqueKeypair
      *
      * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
      **/
-    CHIP_ERROR Delete();
+    CHIP_ERROR DestroyKey();
 
 protected:
     void * mContext      = nullptr;

diff --git a/src/platform/silabs/efr32/Efr32PsaOpaqueKeypair.cpp b/src/platform/silabs/efr32/Efr32PsaOpaqueKeypair.cpp
@@ -124,7 +124,7 @@ EFR32OpaqueKeypair::~EFR32OpaqueKeypair()
         // Delete volatile keys, since nobody else can after we drop the key ID.
         if (!mIsPersistent)
         {
-            Delete();
+            DestroyKey();
         }
 
         MemoryFree(mContext);
@@ -145,7 +145,7 @@ CHIP_ERROR EFR32OpaqueKeypair::Load(EFR32OpaqueKeyId opaque_id)
     // If the object contains a volatile key, clean it up before reusing the object storage
     if (mHasKey && !mIsPersistent)
     {
-        Delete();
+        DestroyKey();
     }
 
     key_id = psa_key_id_from_opaque(opaque_id);
@@ -342,7 +342,7 @@ CHIP_ERROR EFR32OpaqueKeypair::Derive(const uint8_t * their_key, size_t their_ke
     return error;
 }
 
-CHIP_ERROR EFR32OpaqueKeypair::Delete()
+CHIP_ERROR EFR32OpaqueKeypair::DestroyKey()
 {
     CHIP_ERROR error    = CHIP_NO_ERROR;
     psa_status_t status = PSA_ERROR_BAD_STATE;

diff --git a/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.cpp b/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.cpp
@@ -209,7 +209,7 @@ CHIP_ERROR Efr32PsaOperationalKeystore::NewOpKeypairForFabric(FabricIndex fabric
         }
         else
         {
-            mPendingKeypair->Delete();
+            mPendingKeypair->DestroyKey();
             if (id == kEFR32OpaqueKeyIdVolatile)
             {
                 id = kEFR32OpaqueKeyIdUnknown;
@@ -313,9 +313,7 @@ CHIP_ERROR Efr32PsaOperationalKeystore::CommitOpKeypairForFabric(FabricIndex fab
     // There's a good chance we'll need the key again soon
     mCachedKey->Load(id);
 
-    mPendingKeypair         = nullptr;
-    mIsPendingKeypairActive = false;
-    mPendingFabricIndex     = kUndefinedFabricIndex;
+    ResetPendingKey(true /* keepKeyPairInStorage */);
 
     return CHIP_NO_ERROR;
 }
@@ -361,7 +359,7 @@ CHIP_ERROR Efr32PsaOperationalKeystore::RemoveOpKeypairForFabric(FabricIndex fab
     if (id == cachedId)
     {
         // Delete from persistent storage and unload
-        mCachedKey->Delete();
+        mCachedKey->DestroyKey();
         return CHIP_NO_ERROR;
     }
 
@@ -372,7 +370,7 @@ CHIP_ERROR Efr32PsaOperationalKeystore::RemoveOpKeypairForFabric(FabricIndex fab
         return CHIP_ERROR_INTERNAL;
     }
 
-    mCachedKey->Delete();
+    mCachedKey->DestroyKey();
 
     return CHIP_NO_ERROR;
 }

diff --git a/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.h b/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.h
@@ -94,13 +94,17 @@ class Efr32PsaOperationalKeystore : public chip::Crypto::OperationalKeystore
     bool mIsInitialized                      = false;
 
 private:
-    void ResetPendingKey()
+    void ResetPendingKey(bool keepKeyPairInStorage = false)
     {
-        if (mPendingKeypair != nullptr)
+        if (mPendingKeypair != nullptr && !keepKeyPairInStorage)
         {
-            mPendingKeypair->Delete();
-            Platform::Delete(mPendingKeypair);
+            // This removes the PSA Keypair from storage and unloads it
+            // using the EFR32OpaqueKeypair context.
+            // We destroy it when the OperationKeyStore process failed.
+            mPendingKeypair->DestroyKey();
         }
+
+        Platform::Delete(mPendingKeypair);
         mPendingKeypair         = nullptr;
         mIsPendingKeypairActive = false;
         mPendingFabricIndex     = kUndefinedFabricIndex;