Address items in code review

Author: LinuxJedi

.github/workflows/se050-sim.yml

@@ -20,9 +20,9 @@ concurrency:
 # We patch it to COPY the PR checkout instead so CI reflects the PR's source.
 
 env:
-  # Pin the simulator to a known-good revision. Bump this deliberately after
+  # Pin the simulator to a known-good commit. Bump this deliberately after
   # validating upstream changes in a standalone PR.
-  SE050SIM_REF: main
+  SE050SIM_REF: c1474f44a84d8a09278e0ebd83d362d75f1de17b
 
 jobs:
   se050_sim:
@@ -48,7 +48,7 @@ jobs:
         working-directory: se050sim
         run: |
           sed -i 's|^RUN git clone --depth 1 https://github.com/wolfSSL/wolfssl.git /app/wolfssl$|COPY wolfssl /app/wolfssl|' Dockerfile.wolfcrypt
-          # Fail fast if the pattern drifted upstream — better a clear error
+          # Fail fast if the pattern drifted upstream -- better a clear error
           # than a CI run that silently tests master.
           grep -q '^COPY wolfssl /app/wolfssl$' Dockerfile.wolfcrypt
           ! grep -q 'git clone .*wolfssl\.git' Dockerfile.wolfcrypt

wolfcrypt/src/ed25519.c

@@ -1170,9 +1170,10 @@ int wc_ed25519_import_public_ex(const byte* in, word32 inLen, ed25519_key* key,
         return BAD_FUNC_ARG;
 
 #ifdef WOLFSSL_SE050
-    /* Importing new key material invalidates any prior SE050 object binding. */
-    key->keyIdSet = 0;
-    key->keyId    = 0;
+    /* Importing new key material invalidates any prior SE050 object binding;
+     * erase the old object (no-op when keyIdSet == 0) so the host and the
+     * secure element agree on what's bound. */
+    se050_ed25519_free_key(key);
 #endif
 
     /* compressed prefix according to draft
@@ -1262,9 +1263,10 @@ int wc_ed25519_import_private_only(const byte* priv, word32 privSz,
         return BAD_FUNC_ARG;
 
 #ifdef WOLFSSL_SE050
-    /* Importing new key material invalidates any prior SE050 object binding. */
-    key->keyIdSet = 0;
-    key->keyId    = 0;
+    /* Importing new key material invalidates any prior SE050 object binding;
+     * erase the old object (no-op when keyIdSet == 0) so the host and the
+     * secure element agree on what's bound. */
+    se050_ed25519_free_key(key);
 #endif
 
     XMEMCPY(key->k, priv, ED25519_KEY_SIZE);
@@ -1324,9 +1326,14 @@ int wc_ed25519_import_private_key_ex(const byte* priv, word32 privSz,
     }
 
 #ifdef WOLFSSL_SE050
-    /* Importing new key material invalidates any prior SE050 object binding. */
-    key->keyIdSet = 0;
-    key->keyId    = 0;
+    /* Importing new key material invalidates any prior SE050 object binding;
+     * erase the old object (no-op when keyIdSet == 0) so the host and the
+     * secure element agree on what's bound. wc_ed25519_import_public_ex below
+     * does the same reset, but we also do it here explicitly: key->k is
+     * overwritten before that call, so the binding must be dropped first in
+     * case wc_ed25519_import_public_ex fails its own early-return argument
+     * checks before reaching its reset. */
+    se050_ed25519_free_key(key);
 #endif
 
     XMEMCPY(key->k, priv, ED25519_KEY_SIZE);

wolfcrypt/src/port/nxp/se050_port.c

@@ -1540,7 +1540,7 @@ int se050_rsa_verify(const byte* in, word32 inLen, byte* out, word32 outLen,
     if (status == kStatus_SSS_Success) {
         if (keyCreated) {
             /* We uploaded only the public part of the key for this verify.
-             * Don't persist keyIdSet=1 — a later sign on the same RsaKey
+             * Don't persist keyIdSet=1 -- a later sign on the same RsaKey
              * would reuse this binding and fail because the SE050 object has
              * no private material. Erase the transient object so the next
              * SE050 op (sign or verify) re-uploads from whatever the host
@@ -3053,6 +3053,10 @@ int se050_ed25519_verify_msg(const byte* signature, word32 signatureLen,
         key, signature, signatureLen, msg, msgLen);
 #endif
 
+    if (signature == NULL || msg == NULL || key == NULL || res == NULL) {
+        return BAD_FUNC_ARG;
+    }
+
     *res = 0;
 
     if (cfg_se050_i2c_pi == NULL) {
@@ -3115,8 +3119,21 @@ int se050_ed25519_verify_msg(const byte* signature, word32 signatureLen,
     }
 
     if (status == kStatus_SSS_Success) {
-        key->keyId = keyId;
-        key->keyIdSet = 1;
+        if (keyCreated) {
+            /* We uploaded only the public part of the key for this verify.
+             * Don't persist keyIdSet=1 -- a later sign on the same ed25519_key
+             * would reuse this binding and fail because the SE050 object has
+             * no private material. Erase the transient object so the next
+             * SE050 op re-uploads. Mirrors the fix in se050_rsa_verify. */
+            sss_key_store_erase_key(&host_keystore, &newKey);
+            sss_key_object_free(&newKey);
+        }
+        else {
+            /* Pre-existing keyIdSet=1 binding (from prior sign that uploaded
+             * a keypair, or explicit caller setup). Preserve it. */
+            key->keyId = keyId;
+            key->keyIdSet = 1;
+        }
         *res = 1;
         ret = 0;
     }

wolfcrypt/test/test.c

@@ -41060,7 +41060,7 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t ed25519_test(void)
     /* These cases exercise host-side signature-encoding pre-validation (e.g.,
      * sig == curve order). The SE050 port delegates verify to the secure
      * element, which rejects all four inputs with WC_HW_E rather than the
-     * BAD_FUNC_ARG / SIG_VERIFY_E the host-side path produces — so the
+     * BAD_FUNC_ARG / SIG_VERIFY_E the host-side path produces -- so the
      * expected error code differs below when built against an SE050. */
     {
         /* Run tests for some rare code paths */