Skip to content

Commit 5618b8a

Browse files
committed
[TA-100] Address review feedback (Copilot + Fenrir)
Copilot fixes: - atmel.c: ATCA_ENABLE_DEPRECATED I2C path now uses ATECC_I2C_ADDR instead of slave_address=1 (matches the non-deprecated path). - atmel.c: capture and propagate atmel_createHandles() return value; abort init via WC_HW_E if handle creation fails. - atmel.h: include calib_aes_gcm.h with the same <calib/...> form used for calib_command.h so a single -I (.../include or .../include/cryptoauthlib) resolves both. - configure.ac: drop the duplicated AM_CONDITIONAL([BUILD_CRYPTOAUTHLIB]) (kept only in the consolidated section near the end). - settings.h: remove leftover commented-out '#ifdef WOLFSSL_ATECC508A'. - benchmark.c: drop the broken TA100 wc_RsaSSL_Verify branch (it passed message/enc as if they were sig/out). - test.c: stop calling atmel_ecc_free() with the slot-TYPE enum constants; wc_ecc_free(userA/userB) already releases the allocated slots. - ecc.c (microchip_curve_id_for_key): switch on key->dp->id, not size, so SECP256K1 / BRAINPOOLP256R1 are not silently mapped to SECP256R1. Helper is now defined for ATECC508A/608A as well, fixing the TA100-only gating that broke ATECC builds. - ecc.c (_ecc_make_key_ex): keep ATECC508A/608A's curve check at SECP256R1-only (hardware does not support the wider curve set); TA100 retains the multi-curve list. Fenrir fixes: - ecc.c (wc_ecc_init_ex): under TA100 + ALT_ECC_SIZE the pubkey x/y/z pointers must be aimed at key->pubkey.xyz[] (with alt_fp_init) before mp_init_multi - otherwise mp_init_multi dereferenced NULL. - atmel.c (atmel_get_rev_info): check atcab_wakeup return and bail out via atmel_ecc_translate_err before calling atcab_info. - atmel.c (atmel_ecc_create_pms, TA100+ECDH_ENC): pass MAP_TO_HANDLE(slotId) (the ephemeral private-key handle) into talib_ecdh_compat instead of MAP_TO_HANDLE(slotIdEnc). - atmel.c (wc_Microchip_rsa_create_key): on any failure after the first talib_create_element succeeds, delete the previously created handle(s) and clear rKeyH/uKeyH so device elements are not leaked. - aes.c (wc_AesGcmEncrypt / wc_AesGcmDecrypt TA100 fast paths): replace '(authInSz + sz) <= MAX' with bounds on each operand individually so word32 wraparound cannot bypass the 996-byte hardware limit. - rsa.c (RsaPrivateDecryptEx): drop the TA100 RSA_PUBLIC_DECRYPT short-circuit. wc_Microchip_rsa_verify expects (digest, digestLen, sig, sigLen, ...) and the verified flag must be honored; the proper TA100 fast-path already lives in wc_RsaPSS_CheckPadding_ex2.
1 parent ade504a commit 5618b8a

9 files changed

Lines changed: 69 additions & 57 deletions

File tree

configure.ac

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3128,8 +3128,6 @@ AS_IF([test "x$with_cryptoauthlib" != "xno"], [
31283128
])
31293129
])
31303130

3131-
AM_CONDITIONAL([BUILD_CRYPTOAUTHLIB], [test "x$ENABLED_CRYPTOAUTHLIB" = "xyes"])
3132-
31333131
# TropicSquare TROPIC01
31343132
# Example: "./configure --with-tropic01=/home/pi/libtropic"
31353133
ENABLED_TROPIC01="no"

wolfcrypt/benchmark/benchmark.c

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10575,13 +10575,8 @@ static void bench_rsa_helper(int useDeviceID,
1057510575
1, &times, ntimes, &pending)) {
1057610576
#if !defined(WOLFSSL_RSA_VERIFY_INLINE) && \
1057710577
!defined(WOLFSSL_RSA_PUBLIC_ONLY)
10578-
#if defined(WOLFSSL_MICROCHIP_TA100)
10579-
ret = wc_RsaSSL_Verify(message, len,
10580-
enc[i], rsaKeySz/8, rsaKey[i]);
10581-
#else
1058210578
ret = wc_RsaSSL_Verify(enc[i], idx, out[i],
1058310579
rsaKeySz/8, rsaKey[i]);
10584-
#endif
1058510580
#elif defined(USE_CERT_BUFFERS_2048)
1058610581
XMEMCPY(enc[i], rsa_2048_sig, sizeof(rsa_2048_sig));
1058710582
idx = sizeof(rsa_2048_sig);

wolfcrypt/src/aes.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10162,7 +10162,8 @@ int wc_AesGcmEncrypt(Aes* aes, byte* out, const byte* in, word32 sz,
1016210162
aes->keylen == TA_KEY_TYPE_AES128_SIZE &&
1016310163
ivSz == TA_AES_GCM_IV_LENGTH &&
1016410164
authTagSz == TA_AES_GCM_TAG_LENGTH &&
10165-
(authInSz + sz) <= TA_AES_GCM_MAX_DATA_SIZE) {
10165+
sz <= TA_AES_GCM_MAX_DATA_SIZE &&
10166+
authInSz <= (word32)(TA_AES_GCM_MAX_DATA_SIZE - sz)) {
1016610167
return wc_Microchip_AesGcmEncrypt(
1016710168
aes, out, in, sz,
1016810169
iv, ivSz,
@@ -10906,7 +10907,8 @@ int wc_AesGcmDecrypt(Aes* aes, byte* out, const byte* in, word32 sz,
1090610907
aes->keylen == TA_KEY_TYPE_AES128_SIZE &&
1090710908
ivSz == TA_AES_GCM_IV_LENGTH &&
1090810909
authTagSz == TA_AES_GCM_TAG_LENGTH &&
10909-
(authInSz + sz) <= TA_AES_GCM_MAX_DATA_SIZE) {
10910+
sz <= TA_AES_GCM_MAX_DATA_SIZE &&
10911+
authInSz <= (word32)(TA_AES_GCM_MAX_DATA_SIZE - sz)) {
1091010912
return wc_Microchip_AesGcmDecrypt(
1091110913
aes, out, in, sz, iv, ivSz,
1091210914
authTag, authTagSz, authIn, authInSz);

wolfcrypt/src/ecc.c

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5660,16 +5660,15 @@ int wc_ecc_make_pub_ex(ecc_key* key, ecc_point* pubOut, WC_RNG* rng)
56605660
return err;
56615661
}
56625662

5663-
#if defined(WOLFSSL_MICROCHIP_TA100)
5664-
static WC_INLINE int ta100_curve_id_for_key(const ecc_key* key)
5663+
#if defined(WOLFSSL_ATECC508A) || defined(WOLFSSL_ATECC608A) || \
5664+
defined(WOLFSSL_MICROCHIP_TA100)
5665+
/* Resolve the curve id to pass to the Microchip backend. Keep the curve
5666+
* distinction by id (not size): SECP256R1, SECP256K1 and BRAINPOOLP256R1 are
5667+
* all 32 bytes and must NOT be collapsed onto SECP256R1. */
5668+
static WC_INLINE int microchip_curve_id_for_key(const ecc_key* key)
56655669
{
56665670
if (key != NULL && key->dp != NULL) {
5667-
switch (key->dp->size) {
5668-
case 28: return ECC_SECP224R1;
5669-
case 32: return ECC_SECP256R1;
5670-
case 48: return ECC_SECP384R1;
5671-
default: return key->dp->id;
5672-
}
5671+
return key->dp->id;
56735672
}
56745673
return ECC_CURVE_DEF;
56755674
}
@@ -5764,24 +5763,21 @@ static int _ecc_make_key_ex(WC_RNG* rng, int keysize, ecc_key* key,
57645763

57655764
#if defined(WOLFSSL_ATECC508A) || defined(WOLFSSL_ATECC608A) || \
57665765
defined(WOLFSSL_MICROCHIP_TA100)
5767-
#if !defined(WOLFSSL_MICROCHIP_TA100)
5766+
#if defined(WOLFSSL_MICROCHIP_TA100)
5767+
/* TA100 supports multiple curves natively. */
57685768
if (key->dp->id == ECC_SECP256R1 ||
57695769
key->dp->id == ECC_SECP224R1 ||
57705770
key->dp->id == ECC_SECP384R1 ||
57715771
key->dp->id == ECC_SECP256K1 ||
57725772
key->dp->id == ECC_BRAINPOOLP256R1) {
5773-
/* supports more than ECC256R1 curve */
57745773
#else
5775-
if (key->dp->id == ECC_SECP256R1 ||
5776-
key->dp->id == ECC_SECP224R1 ||
5777-
key->dp->id == ECC_SECP384R1 ||
5778-
key->dp->id == ECC_SECP256K1 ||
5779-
key->dp->id == ECC_BRAINPOOLP256R1) {
5774+
/* ATECC508A/608A hardware only supports SECP256R1. */
5775+
if (key->dp->id == ECC_SECP256R1) {
57805776
#endif
57815777
key->type = ECC_PRIVATEKEY;
57825778
if (key->slot == ATECC_INVALID_SLOT)
57835779
key->slot = atmel_ecc_alloc(ATMEL_SLOT_ECDHE);
5784-
err = atmel_ecc_create_key(key->slot, ta100_curve_id_for_key(key),
5780+
err = atmel_ecc_create_key(key->slot, microchip_curve_id_for_key(key),
57855781
key->pubkey_raw);
57865782

57875783
/* populate key->pubkey */
@@ -6282,13 +6278,24 @@ int wc_ecc_init_ex(ecc_key* key, void* heap, int devId)
62826278
defined(WOLFSSL_MICROCHIP_TA100)
62836279
key->slot = ATECC_INVALID_SLOT;
62846280
#ifdef WOLFSSL_MICROCHIP_TA100
6285-
/* TA100 needs pubkey initialized to populate after genkey */
6281+
/* TA100 needs pubkey initialized to populate after genkey. With
6282+
* ALT_ECC_SIZE the x/y/z pointers must first be aimed at the inline
6283+
* xyz[] storage; mp_init_multi otherwise dereferences NULL. */
6284+
#ifdef ALT_ECC_SIZE
6285+
key->pubkey.x = (mp_int*)&key->pubkey.xyz[0];
6286+
key->pubkey.y = (mp_int*)&key->pubkey.xyz[1];
6287+
key->pubkey.z = (mp_int*)&key->pubkey.xyz[2];
6288+
alt_fp_init(key->pubkey.x);
6289+
alt_fp_init(key->pubkey.y);
6290+
alt_fp_init(key->pubkey.z);
6291+
#else
62866292
ret = mp_init_multi(key->pubkey.x, key->pubkey.y, key->pubkey.z,
62876293
NULL, NULL, NULL);
62886294
if (ret != MP_OKAY) {
62896295
return MEMORY_E;
62906296
}
62916297
#endif
6298+
#endif
62926299
#else
62936300
#if defined(WOLFSSL_KCAPI_ECC)
62946301
key->handle = NULL;
@@ -6531,14 +6538,14 @@ static int wc_ecc_sign_hash_hw(const byte* in, word32 inlen,
65316538
#if defined(WOLFSSL_ATECC508A) || defined(WOLFSSL_ATECC608A) || \
65326539
defined(WOLFSSL_MICROCHIP_TA100)
65336540
#if defined(WOLFSSL_MICROCHIP_TA100)
6534-
if (ta100_curve_id_for_key(key) == ECC_SECP256R1) {
6541+
if (microchip_curve_id_for_key(key) == ECC_SECP256R1) {
65356542
(void)inlen;
65366543
/* Sign: Result is 32-bytes of R then 32-bytes of S */
65376544
err = atmel_ecc_sign(key->slot, in, out);
65386545
}
65396546
else {
65406547
/* Sign: Result is raw R||S */
6541-
err = atmel_ecc_sign_ex(key->slot, ta100_curve_id_for_key(key),
6548+
err = atmel_ecc_sign_ex(key->slot, microchip_curve_id_for_key(key),
65426549
in, inlen, out);
65436550
}
65446551
#else
@@ -9422,7 +9429,7 @@ int wc_ecc_verify_hash_ex(mp_int *r, mp_int *s, const byte* hash,
94229429
#endif /* WOLFSSL_SE050 */
94239430

94249431
#if defined(WOLFSSL_MICROCHIP_TA100)
9425-
if (ta100_curve_id_for_key(key) == ECC_SECP256R1) {
9432+
if (microchip_curve_id_for_key(key) == ECC_SECP256R1) {
94269433
err = atmel_ecc_verify(hash, sigRS, key->pubkey_raw, res);
94279434
if (err != 0) {
94289435
return err;
@@ -9431,7 +9438,7 @@ int wc_ecc_verify_hash_ex(mp_int *r, mp_int *s, const byte* hash,
94319438
}
94329439
else {
94339440
err = atmel_ecc_verify_ex(hash, hashlen, sigRS, key->pubkey_raw,
9434-
keySz * 2, ta100_curve_id_for_key(key), res);
9441+
keySz * 2, microchip_curve_id_for_key(key), res);
94359442
if (err != 0) {
94369443
return err;
94379444
}

wolfcrypt/src/port/atmel/atmel.c

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ static int ateccx08a_cfg_initialized = 0;
147147
.devtype = MICROCHIP_DEV_TYPE,
148148
.atcai2c = {
149149
#ifdef ATCA_ENABLE_DEPRECATED
150-
.slave_address = 1,
150+
.slave_address = ATECC_I2C_ADDR,
151151
#else
152152
.address = ATECC_I2C_ADDR,
153153
#endif
@@ -578,11 +578,12 @@ int atmel_get_rev_info(word32* revision)
578578
WOLFSSL_MSG("Waking device...");
579579
#endif
580580
ret = atcab_wakeup();
581+
if (ret != ATCA_SUCCESS) {
581582
#ifdef WOLFSSL_ATECC_DEBUG
582-
if (ret != 0) {
583583
WOLFSSL_MSG("atcab_wakeup failed");
584-
}
585584
#endif
585+
return atmel_ecc_translate_err(ret);
586+
}
586587
ret = atcab_info((uint8_t*)revision);
587588
ret = atmel_ecc_translate_err(ret);
588589
return ret;
@@ -616,8 +617,11 @@ int atmel_ecc_create_pms(int slotId, const uint8_t* peerKey, uint8_t* pms)
616617

617618
#ifdef WOLFSSL_ATECC_ECDH_ENC
618619
#ifdef WOLFSSL_MICROCHIP_TA100
619-
(void)slotId;
620-
ret = talib_ecdh_compat(atcab_get_device(), MAP_TO_HANDLE(slotIdEnc),
620+
/* TA100 ECDH uses the ephemeral private-key slot; the encryption
621+
* parent slot is allocated above only for parity with the
622+
* non-TA100 atcab_ecdh_enc path and is not consumed here. */
623+
(void)slotIdEnc;
624+
ret = talib_ecdh_compat(atcab_get_device(), MAP_TO_HANDLE(slotId),
621625
peerKey, pms);
622626
#else
623627
/* send the encrypted version of the ECDH command */
@@ -912,24 +916,34 @@ int wc_Microchip_rsa_create_key(struct RsaKey* key, int size, long e)
912916
ret = talib_handle_init_public_key(&uKeyA, WOLFSSL_TA_KEY_TYPE_RSA,
913917
TA_ALG_MODE_RSA_SSA_PSS, 0, 0);
914918
if (ret != ATCA_SUCCESS)
915-
return WC_HW_E;
919+
goto err_free_r;
916920

917921
ta100_fix_property_endian(&uKeyA);
918922

919923
ret = talib_create_element(atcab_get_device(), &uKeyA, &key->uKeyH);
920924
if (ret != ATCA_SUCCESS)
921-
return WC_HW_E;
925+
goto err_free_r;
922926

923927
ret = talib_genkey_base(atcab_get_device(), TA_KEYGEN_MODE_NEWKEY,
924928
(uint32_t)key->rKeyH, key->uKey, &uKey_len);
925929
if (ret != ATCA_SUCCESS)
926-
return WC_HW_E;
930+
goto err_free_ru;
927931

928932
/* Use talib_write_element, not talib_write_pub_key */
929933
ret = talib_write_element(atcab_get_device(), key->uKeyH,
930934
(uint16_t)uKey_len, key->uKey);
935+
if (ret != ATCA_SUCCESS)
936+
goto err_free_ru;
931937

932938
return atmel_ecc_translate_err(ret);
939+
940+
err_free_ru:
941+
(void)talib_delete_handle(atcab_get_device(), (uint32_t)key->uKeyH);
942+
key->uKeyH = 0;
943+
err_free_r:
944+
(void)talib_delete_handle(atcab_get_device(), (uint32_t)key->rKeyH);
945+
key->rKeyH = 0;
946+
return WC_HW_E;
933947
}
934948

935949
int wc_Microchip_rsa_encrypt(const byte* in, word32 inLen, byte* out,
@@ -1245,7 +1259,10 @@ int atmel_init(void)
12451259
}
12461260
#ifdef WOLFSSL_MICROCHIP_TA100
12471261
/* create handles for TA100 */
1248-
atmel_createHandles();
1262+
if (atmel_createHandles() != 0) {
1263+
WOLFSSL_MSG("atmel_createHandles failed");
1264+
return WC_HW_E;
1265+
}
12491266
#endif
12501267
}
12511268

wolfcrypt/src/rsa.c

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3833,18 +3833,11 @@ static int RsaPrivateDecryptEx(const byte* in, word32 inLen, byte* out,
38333833
}
38343834
return WC_HW_E;
38353835
}
3836-
else if (rsa_type == RSA_PUBLIC_DECRYPT &&
3837-
pad_value == RSA_BLOCK_TYPE_1) {
3838-
if (key->uKeyH != 0) {
3839-
int tmp;
3840-
if (pad_type != WC_RSA_PSS_PAD) {
3841-
return WC_HW_E;
3842-
}
3843-
return wc_Microchip_rsa_verify(in, inLen,
3844-
out, outLen, key, &tmp);
3845-
}
3846-
return WC_HW_E;
3847-
}
3836+
/* Note: RSA_PUBLIC_DECRYPT (verify) is intentionally not intercepted
3837+
* here. wc_Microchip_rsa_verify takes a digest as input, not a raw
3838+
* signature blob; the proper TA100 short-circuit lives in the
3839+
* wc_RsaPSS_CheckPadding / wc_RsaPSS_VerifyCheck path which has the
3840+
* digest available. */
38483841
#elif defined(WOLFSSL_SE050) && !defined(WOLFSSL_SE050_NO_RSA)
38493842
if (rsa_type == RSA_PRIVATE_DECRYPT && pad_value == RSA_BLOCK_TYPE_2) {
38503843
ret = se050_rsa_private_decrypt(in, inLen, out, outLen, key,

wolfcrypt/test/test.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37101,10 +37101,8 @@ static wc_test_ret_t ecc_test_curve_size(WC_RNG* rng, int keySize, int testVerif
3710137101
WC_FREE_VAR(sig, HEAP_HINT);
3710237102
WC_FREE_VAR(digest, HEAP_HINT);
3710337103
#endif
37104-
#if defined(WOLFSSL_MICROCHIP_TA100)
37105-
atmel_ecc_free(ATMEL_SLOT_ECDHE_ALICE);
37106-
atmel_ecc_free(ATMEL_SLOT_ECDHE_BOB);
37107-
#endif
37104+
/* Slot cleanup happens via wc_ecc_free(userA/userB) above; do not call
37105+
* atmel_ecc_free with the slot-type enum constants. */
3710837106
(void)keySize;
3710937107
(void)curve_id;
3711037108
(void)rng;

wolfssl/wolfcrypt/port/atmel/atmel.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@
3939
#include <calib/calib_command.h>
4040
#endif
4141
#if defined(WOLFSSL_MICROCHIP_TA100) && defined(HAVE_AESGCM)
42-
#include <cryptoauthlib/calib/calib_aes_gcm.h>
42+
/* Use the same include style as <calib/calib_command.h> above so the
43+
* single -I added by configure.ac (either .../include or
44+
* .../include/cryptoauthlib) resolves both. */
45+
#include <calib/calib_aes_gcm.h>
4346
#endif
4447
#ifndef ATECC_MAX_SLOT
4548
#define ATECC_MAX_SLOT (0x8) /* Only use 0-7 */

wolfssl/wolfcrypt/settings.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1227,7 +1227,6 @@
12271227
#endif
12281228
#endif
12291229

1230-
/*#ifdef WOLFSSL_ATECC508A*/
12311230
#if defined(WOLFSSL_ATECC508A) || \
12321231
defined(WOLFSSL_MICROCHIP_TA100)
12331232
/* backwards compatibility */

0 commit comments

Comments
 (0)