Skip to content

Add Altera Agilex 5 SDM crypto offload port - #11042

Open
aidangarske wants to merge 12 commits into
wolfSSL:masterfrom
aidangarske:agilex5-fcs-port
Open

aidangarske wants to merge 12 commits into
wolfSSL:masterfrom
aidangarske:agilex5-fcs-port

Conversation

@aidangarske

@aidangarske aidangarske commented Aug 4, 2026

Copy link
Copy Markdown
Member

Description

  • Adds libfcs based Agilex 5 SDM crypto callback integration
  • Offloads RNG SHA256 AES ECC and HMAC operations
  • Keeps unsupported and short operations on safe software fallback paths
  • Preserves device resident ECC key isolation for signing and key agreement
  • Adds lifecycle rekey copy cleanup and hardware usage regression coverage
  • Supports Autotools CMake and user settings configurations
  • Validates the BitBake packaged wolfCrypt test on Agilex 5 hardware
  • Requires the matching meta-wolfSSL integration for the documented image flow

partner pr here: wolfSSL/meta-wolfssl#176

Copilot AI review requested due to automatic review settings August 4, 2026 01:01
@aidangarske
aidangarske marked this pull request as ready for review August 4, 2026 01:06
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

retest this please

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new wolfCrypt hardware-acceleration port for the Altera Agilex 5 Secure Device Manager (SDM) via libfcs, wiring it into wolfCrypt’s crypto callback framework with software fallback behavior and CI stubs.

Changes:

  • Introduces the Altera FCS port (session glue + crypto callback dispatcher + RNG/SHA-256/AES/ECC/HMAC verify integration) and associated public header/docs.
  • Integrates the port into Autotools and CMake builds (options, defines, sources, link requirements) and adds a CI workflow using stubbed libfcs/libfdt.
  • Updates crypto callback lifecycle handling (cleanup returns status; cleanup can defer/unwind when callbacks report BUSY_E) and adds regression coverage in wolfcrypt tests.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
wolfssl/wolfcrypt/settings.h Auto-enables crypto-callback capabilities needed by the FCS port (CMD/SETKEY).
wolfssl/wolfcrypt/port/altera/altera_fcs.h Adds the public Altera FCS port header/API surface and compile-time guards.
wolfssl/wolfcrypt/include.am Installs the new Altera FCS port header.
wolfssl/wolfcrypt/cryptocb.h Updates crypto callback cleanup API to return status.
wolfcrypt/test/test.c Adds ALTERA-FCS hardware/no-hardware tests and a BUSY_E unregister regression test.
wolfcrypt/src/wc_port.c Auto-registers/unregisters the FCS callback in wolfCrypt_Init/Cleanup with BUSY handling.
wolfcrypt/src/port/altera/README.md Documents build, behavior, limitations, and validation for the Altera FCS port.
wolfcrypt/src/port/altera/altera_fcs_rng.c Implements SDM TRNG seeding path for wolfCrypt RNG via libfcs.
wolfcrypt/src/port/altera/altera_fcs_hmac.c Adds explicit API for SDM “vault-key” HMAC verification.
wolfcrypt/src/port/altera/altera_fcs_hash.c Implements buffered SHA offload with mandatory copy/free callbacks and software fallback.
wolfcrypt/src/port/altera/altera_fcs_glue.c Implements process-wide session management, locking, key-id allocation, and atfork behavior.
wolfcrypt/src/port/altera/altera_fcs_ecc.c Implements device-resident ECC key creation and ECDSA/ECDH operations with no silent fallback.
wolfcrypt/src/port/altera/altera_fcs_cryptocb.c Implements the crypto-callback dispatcher, registration mask, and deferred unregister logic.
wolfcrypt/src/port/altera/altera_fcs_aes.c Implements AES-CBC/AES-CTR offload with key import/retirement and strict eligibility rules.
wolfcrypt/src/include.am Adds Altera FCS sources to distribution and conditional build sources.
wolfcrypt/src/cryptocb.c Makes crypto callback cleanup return status and honors BUSY_E on unregister command.
tests/include.am Adds stubbed libfcs/libfdt files to the distribution list.
tests/altera-fcs-stub/libfcs.h Provides minimal libfcs API header for host CI builds.
tests/altera-fcs-stub/libfcs_stub.c Stub implementation that reports unavailable hardware (CI fallback validation).
tests/altera-fcs-stub/libfcs_osal_types.h Stub OSAL type definitions needed by the libfcs header.
tests/altera-fcs-stub/fdt_stub.c Link-only stub for libfdt dependency in CI.
configure.ac Adds --enable-alterafcs option and wires feature selection/defines/libs into Autotools.
CMakeLists.txt Adds WOLFSSL_ALTERA_FCS option and finds/lib-links libfcs + libfdt for CMake builds.
cmake/options.h.in Adds CMake-generated preprocessor defines for Altera FCS-related options.
cmake/functions.cmake Adds BUILD_ALTERA_FCS* feature derivation and source-list wiring for CMake builds.
.github/workflows/altera-fcs.yml Adds CI workflow building/testing the port against the stubbed dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread wolfcrypt/src/port/altera/altera_fcs_glue.c Outdated
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

gcc-arm-cortex-m3

  • FLASH: .text +512 B (+0.4%, 126,523 B / 262,144 B, total: 48% used)

gcc-arm-cortex-m4

  • FLASH: .text +64 B (+0.0%, 205,213 B / 262,144 B, total: 78% used)

gcc-arm-cortex-m4-openssl-compat

  • FLASH: .rodata +32 B, .text +768 B (+0.1%, 785,252 B / 1,048,576 B, total: 75% used)

gcc-arm-cortex-m4-pkcs7

  • FLASH: .text +64 B (+0.0%, 217,689 B / 262,144 B, total: 83% used)

gcc-arm-cortex-m4-rsa-only

  • FLASH: .text +768 B (+0.2%, 334,112 B / 1,048,576 B, total: 32% used)

gcc-arm-cortex-m4-tls12

  • FLASH: .text +512 B (+0.4%, 127,283 B / 262,144 B, total: 49% used)

gcc-arm-cortex-m7

  • FLASH: .text +64 B (+0.0%, 205,213 B / 262,144 B, total: 78% used)

gcc-arm-cortex-m7-pq

  • FLASH: .text +64 B (+0.0%, 303,624 B / 1,048,576 B, total: 29% used)

linuxkm-pie

  • Data: __patchable_function_entries +40 B (+0.1%, 27,144 B)

linuxkm-standard

@aidangarske

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #11042

Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

Findings: 19
19 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/test/test.c Outdated
Comment thread wolfcrypt/test/test.c
Comment thread wolfcrypt/test/test.c
Comment thread wolfssl/wolfcrypt/settings.h
Comment thread wolfcrypt/test/test.c
Comment thread wolfcrypt/test/test.c
Comment thread wolfcrypt/test/test.c
Comment thread wolfcrypt/test/test.c Outdated
Comment thread wolfcrypt/test/test.c
@aidangarske

Copy link
Copy Markdown
Member Author

Jenkins retest this please "logs lost"

@philljj

philljj commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Retest this please.

(no logs)

@aidangarske

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@night1rider night1rider left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for this taking longer than I wanted, got caught up with other work and had issues getting my Agilex 5 in a state that I could do some testing. Please reach out if you have any questions about these review comments.

Comment on lines +71 to +72
#define FCS_KEY_OBJ_MAGIC 0x43736B4FU
#define FCS_KEY_DATA_MAGIC 0x43736B64U

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on what this "magic" represents would be helpful.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are ASCII "CskO" and "Cskd" from the fcs_prepare key object layout. I added a note about it

return 0;
}

static int wc_AlteraFcs_KeyRemove(word32 keyId)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda of a nit: wc_AlteraFcs_KeyRemove() (aes.c:185) and wc_AlteraFcs_EccKeyRemove() (ecc.c:212) have identical bodies that only hand their argument to wc_AlteraFcs_RemoveServiceKey() and return its result. The HMAC path does the same job by calling wc_AlteraFcs_RemoveServiceKey() directly (hmac.c:325), so two of the three algorithms wrap it and one does not. Could you either have them call the function directly or add a wrapper for the hmac. I think I would prefer if they just call the function directly, unless you have a specific reason behind this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped both wrappers, callers use wc_AlteraFcs_RemoveServiceKey and wc_AlteraFcs_DiscardServiceKey directly.

Comment on lines +141 to +183
static int wc_AlteraFcs_KeyObject(byte* out, word32 keyId, const byte* key,
int keyLen, word32* outSz)
{
word32 objSz;
word32 padded;
word32 sizeCode;

if (keyLen == AES_128_KEY_SIZE) {
sizeCode = FCS_KEY_SIZE_128;
}
else if (keyLen == AES_256_KEY_SIZE) {
sizeCode = FCS_KEY_SIZE_256;
}
else {
/* The key object has no code for 192 bit keys. */
return CRYPTOCB_UNAVAILABLE;
}

padded = (word32)keyLen;
if ((padded % WC_ALTERA_FCS_AES_ALIGN) != 0) {
padded += WC_ALTERA_FCS_AES_ALIGN -
(padded % WC_ALTERA_FCS_AES_ALIGN);
}

XMEMSET(out, 0, WC_ALTERA_FCS_KEYOBJ_SZ);
wc_AlteraFcs_Put32(out, FCS_KEY_OBJ_MAGIC);
wc_AlteraFcs_Put32(out + 8, keyId);
wc_AlteraFcs_Put32(out + 20, (sizeCode << 16) |
((word32)FCS_KEY_TYPE_AES << 24));
wc_AlteraFcs_Put32(out + 24, FCS_KEY_USAGE_ENC_DEC);
wc_AlteraFcs_Put32(out + 48, FCS_KEY_DATA_MAGIC);
if (key != NULL) {
XMEMCPY(out + FCS_KEY_DATA_OFFSET, key, (word32)keyLen);
}

/* The declared size covers the object but not the trailing MAC field. */
objSz = FCS_KEY_DATA_OFFSET + padded;
wc_AlteraFcs_Put32(out + 4, ((word32)FCS_KEY_OBJ_VER << 16) |
(objSz & 0xFFFF));

*outSz = objSz + FCS_KEY_MAC_SZ;
return 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wc_AlteraFcs_Put32() is byte for byte identical in all three files, and the three key-object builders write the same six fields at the same six offsets with the same size and padding maths, differing only in which size codes are legal, the type value, the usage value, and whether key bytes are copied. Six constants that describe the header are each defined three times, so the SDM key object layout is recorded in three independent places.

Where the duplicates are

What altera_fcs_aes.c altera_fcs_ecc.c altera_fcs_hmac.c
FCS_KEY_OBJ_MAGIC 71 64 109
FCS_KEY_DATA_MAGIC 72 65 110
FCS_KEY_OBJ_VER 73 66 111
FCS_KEY_MAC_SZ 78 69 113
FCS_KEY_DATA_OFFSET 79 70 114
FCS_KEY_STATUS_SZ 80 72 116
FCS_KEY_ALIGN -- 71 115
FCS_KEY_USAGE_SIGN_VERIFY -- 76 120
wc_AlteraFcs_Put32() 130-136 127-133 129-135
key-object builder 141-183 174-209 176-220
context lookup 105 (AesCtx) 109 (EccCtx) --

wc_AlteraFcs_EccKeyObject() is already the most general of the three, taking keyType and usage as parameters, so it is close to serving all three if it also copied key bytes when given them.

Maybe have a generic _utils file or use the glue file to pull out common functionality?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved Put32/Get32, the key object builder and the header constants into altera_fcs_glue.c and altera_fcs.h. fcs_prepare uses keyBits/128 as the size code for every key type so one builder serves AES, ECC and HMAC.

word32 keyId; /* SDM service key slot id */
int keyLen;
int origin; /* WC_ALTERA_FCS_AES_IMPORTED | _RESIDENT */
byte key[AES_256_KEY_SIZE]; /* imported origin only; zero for resident */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AlteraAesKey holds the plaintext key in keyCtx->key (line 101), copied in at line 282 and kept for the life of the context, and the only thing that reads it is the ConstantCompare() at line 217 which checks the tracked keyId still matches the current key material. Since the port already requires WOLF_CRYPTO_CB_SETKEY (line 65) and wolfSSL calls wc_CryptoCb_SetKey() on every wc_AesSetKey() (aes.c:5100 and :5711), a real re-key has already freed the context through wc_AlteraFcs_AesSetKey() before that compare can run. Is the copy still needed, and if some path can change aes->devKey without the callback firing, would a digest of the key material serve the same purpose without keeping key bytes?

AES is the only one of the three that does this. HMAC imports caller-supplied plaintext keys the same way but the bytes only reach a local key object that is wiped with ForceZero() at altera_fcs_hmac.c:298, leaving the caller holding just a keyId, and AlteraEccKey has no key field at all. Resident AES keys are also unaffected, since the field is left zero for them, and both AES free paths ForceZero the context at lines 236 and 515.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I removed the copy and the compare, the struct holds no key bytes now. Every re-key on this target runs the SETKEY hook first (AesSetKeyLocal_body in aes.c), so the context is freed before that compare could be reached.

* offloaded by handle with no software fallback. Requires
* wc_AesInit(aes, heap, WOLFSSL_ALTERA_FCS_DEVID) first and an empty devCtx.
* Do not call wc_AesSetKey on the resulting context. */
int wc_AlteraFcsAes_MakeKey(Aes* aes, int keyBits)

@night1rider night1rider Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not personally a fan of adding device specific functions to the public api.

Would a second devId, say WOLFSSL_ALTERA_FCS_AES_KEY_DEVID, be worth adding so a device key can be created through the standard wc_AesSetKey() rather than the Altera-only wc_AlteraFcsAes_MakeKey()? The devId is what asks for a key to be generated, and the port's existing SETKEY handler would then require userKey to be NULL as confirmation, erroring out if it is not, so that a caller who passed real key material can never have it silently discarded in favour of a generated one. A caller would init with that devId and call wc_AesSetKey(&aes, NULL, 32, iv, dir), where the length says 128 or 256, and the handler would create the key in the device, set aes->devId to the normal WOLFSSL_ALTERA_FCS_DEVID and return 0. Any context left on the key-generation devId outside that one path would fail rather than quietly do something else.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense added a WOLFSSL_ALTERA_FCS_AES_KEY_DEVID (0x4144) takes a NULL key in wc_AesSetKey, creates the resident key and moves the context to the main devId, and wc_AlteraFcsAes_MakeKey is gone.

Comment on lines +268 to +272
/* pthread_atfork protects threaded builds from inheriting a locked mutex.
* The PID check also covers SINGLE_THREADED builds. */
if (g_ownerPid != 0 && g_ownerPid != getpid()) {
wc_AlteraFcs_ResetForkChild();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wc_AlteraFcs_SessionAcquire() compares g_ownerPid against getpid() and may call wc_AlteraFcs_ResetForkChild(), which clears the session, refcount, orphan list and lock descriptor, all before wc_LockMutex(&g_lock) is reached. The comment says pthread_atfork covers the threaded case and that is fair, but the read and the reset are still unprotected. The same pattern appears again at line 564 in wc_AlteraFcs_Cleanup(), where it does run under the mutex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the pid check and reset under g_lock inside wc_AlteraFcs_Init and dropped the duplicate in SessionAcquire, matching the one in Cleanup.


/* Oversized messages exceed a single SDM transaction, so they are
* finished in software rather than rejected by the driver. */
if (hwOk && msgLen >= WOLFSSL_ALTERA_FCS_HASH_MIN &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOLFSSL_ALTERA_FCS_HASH_MIN also defaults to 4096, so any message shorter than that is never offered to the device and is finished by wc_AlteraFcs_SoftDigest() instead. So unless the chip has a minimum input, I dont think we should be silently defaulting to software here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I removed The only hardware rule that showed up on the board is that the SDM rejects lengths that are not a multiple of 8 bytes (one shot errors, streaming returns a wrong digest), so those finish on the software shadow and everything else goes to the device.

/* Oversized messages exceed a single SDM transaction, so they are
* finished in software rather than rejected by the driver. */
if (hwOk && msgLen >= WOLFSSL_ALTERA_FCS_HASH_MIN &&
msgLen <= WC_ALTERA_FCS_MAX_XFER) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The port only ever calls the one shot entry points and never fcs_get_digest_streaming(), fcs_aes_crypt_streaming() or fcs_mac_verify_streaming(), which is what forces both the 4 MiB single transaction ceiling and the buffering that ends in a software digest. Those streaming calls take file paths rather than buffers, but memfd_create() creates an anonymous file that lives entirely in memory and is referenced by path as /proc/self/fd/N, so nothing has to be written to storage to use them. Between dropping the minimum and using the streaming path, the device could serve every size rather than only messages that happen to land between 4096 bytes and 4 MiB.

memfd_create() is Linux only rather than standard C, but so is the rest of the port, which already uses flock() from sys/file.h, getpid() and pthread_atfork(), and libfcs itself is a Linux userspace library talking to a Linux kernel driver.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay done, The message accumulates in a memfd and final calls fcs_get_digest_streaming on /proc/self/fd/N, AES and HMAC verify do the same past 4 MiB, and all three were checked across the chunk boundary on the board.

Comment on lines +134 to +138
if (info->algo_type == WC_ALGO_TYPE_NONE) {
if (info->cmd.type == WC_CRYPTOCB_CMD_TYPE_UNREGISTER) {
ret = wc_AlteraFcsCryptoCb_UnRegisterDeviceEx(devId);
return (ret == 0) ? 0 : BUSY_E;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (ret == 0) ? 0 : BUSY_E; turns every failure from wc_AlteraFcsCryptoCb_UnRegisterDeviceEx() into BUSY_E, whatever actually went wrong. That matters more than a usual error squash because wc_CryptoCb_Cleanup() treats BUSY_E as a reason to stop the teardown loop, so an unrelated failure would halt cleanup for every other registered device. Passing the real error through, and reserving BUSY_E for a device that is genuinely busy, would keep that signal meaningful.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Fixed

Comment on lines +134 to +141
if (info->algo_type == WC_ALGO_TYPE_NONE) {
if (info->cmd.type == WC_CRYPTOCB_CMD_TYPE_UNREGISTER) {
ret = wc_AlteraFcsCryptoCb_UnRegisterDeviceEx(devId);
return (ret == 0) ? 0 : BUSY_E;
}
return CRYPTOCB_UNAVAILABLE;
}
#endif

@night1rider night1rider Sep 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wc_CryptoCb_RegisterDevice() already calls the callback with WC_CRYPTOCB_CMD_TYPE_REGISTER as an init hook, and already unwinds on failure by calling wc_CryptoCb_ClearDev(dev) and returning the error, so a port can do its device setup there and have a failed setup undo the registration for free. The port handles WC_CRYPTOCB_CMD_TYPE_UNREGISTER but returns CRYPTOCB_UNAVAILABLE for REGISTER at line 139, which wolfSSL reads as "not implemented", and instead does the registration from wolfCrypt_Init() with the hand written WOLFCRYPT_INIT_FCS_ROLLBACK macro. Can the device setup happen in the REGISTER handler, where the framework already provides the hook and the unwinding, which would leave no Altera specific code in wc_port.c at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved wc_AlteraFcs_Init and the state setup into the REGISTER handler so a failed setup unwinds inside wc_CryptoCb_RegisterDevice, and dropped the rollback macro. wolfCrypt_Init keeps one plain register call like the Versal ASU one above it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the good feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants