Conversation
|
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 12 total — 12 posted, 0 skipped
12 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] wc_HWPUF_Register reads uninitialized hwpuf-registered before zeroing —
wolfcrypt/src/hwpuf.c:42-65 - [Medium] Unused static function getACFromPFR (dead code, uses memset) —
wolfcrypt/src/port/nxp/hwpuf_port.c:54-65 - [Medium] nxp_hwpuf_SetKey has multiple unused parameters —
wolfcrypt/src/port/nxp/hwpuf_port.c:185-195 - [Medium] Callback parameter ctx shadows file-scope global ctx and is unused —
wolfcrypt/src/port/nxp/hwpuf_port.c:258 - [Medium] RNG hardware register read without ensuring RNG is initialized —
wolfcrypt/src/port/nxp/hwpuf_port.c:96 - [Low] Signed/unsigned comparison in hwpuf_test loop —
wolfcrypt/test/test.c:23443-23444 - [Low] WOLFSSL_NXP_HWPUF does not imply WOLFSSL_HWPUF —
wolfssl/wolfcrypt/settings.h:2207-2209 - [Low] Inconsistent key-index bound: literal 16 vs kPUF_KeyIndexMax —
wolfcrypt/src/port/nxp/hwpuf_port.c:74-79 - [Low] wc_CryptoInfo.hwpuf.hwpuf typed as void loses type safety* —
wolfssl/wolfcrypt/cryptocb.h:582 - [Low] HWPUF_DEINIT_E error code defined but never returned —
wolfssl/wolfcrypt/error-crypt.h:332 - [Low] No test coverage for SetKey path or double-register —
wolfcrypt/test/test.c:23341-23502 - [Info] hwpuf_test step numbering skips 'Test 6' —
wolfcrypt/test/test.c:23407-23434
Review generated by Skoll
|
Jenkins retest this please. History lost |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 14 total — 14 posted, 0 skipped
11 finding(s) posted as inline comments (see file-level comments below)
3 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] [review] Bad-arg / error-path test coverage gaps for new HWPUF API —
wolfcrypt/test/test.c:23450-23482 - [Low] [review+review-security] Large stack frame in hwpuf_test on embedded target (no WOLFSSL_SMALL_STACK) —
wolfcrypt/test/test.c:23340-23360 - [Low] [review-security] Retrieved key buffer not zeroized on PUF_GetKey failure path —
wolfcrypt/src/port/nxp/hwpuf_port.c:221-225 - [Low] [review-security] ctx keyMask wiped with XMEMSET instead of ForceZero in Enroll —
wolfcrypt/src/port/nxp/hwpuf_port.c:133-134 - [Low] [review-security] hw-bus GetKey path zeroes caller buffer using unvalidated keySz —
wolfcrypt/src/port/nxp/hwpuf_port.c:208-220 - [Low] [review-security] GetHwpufTypeStr may return NULL passed to printf %s —
wolfcrypt/src/cryptocb.c:250-271,373-378 - [Info] [review] Missing space before '=' in HWPUF_GENERATE_KEY_E enumerator —
wolfssl/wolfcrypt/error-crypt.h:332 - [Info] [review] keyCodeCheck custom return compared against NXP kStatus_Success —
wolfcrypt/src/port/nxp/hwpuf_port.c:63-78,201-203 - [Info] [review] New brace-scope created in test just to declare a loop index —
wolfcrypt/test/test.c:23442-23448 - [Info] [review-security] Misleading test comment references wrong variable —
wolfcrypt/test/test.c:23460-23467 - [Info] [review] wc_CryptoInfo.hwpuf.ctx field declared but never populated —
wolfssl/wolfcrypt/cryptocb.h:585
Findings not tied to a diff line
Singleton global state conflicts with per-context wc_HWPUF* API
File: wolfcrypt/src/hwpuf.c:41; wolfcrypt/src/port/nxp/hwpuf_port.c:56-57,80
Function: wc_HWPUF_Register / wc_HWPUF_Unregister / nxp_hwpuf_*
Severity: Medium
Category: api
The public API is shaped around a per-instance wc_HWPUF* context, but registration/device state is held in file-scope statics: hwpuf_registered in hwpuf.c, and ctx, conf, nxp_rng_initialized in the NXP port. This makes the implementation effectively a singleton. Two consequences: (1) wc_HWPUF_Register on a second context returns HWPUF_REGISTER_E even though it is a distinct object; (2) wc_HWPUF_Unregister(ctxB) while ctxA is the registered context still passes the if (!hwpuf_registered) guard (the global is 1) and calls nxp_hwpuf_UnregisterDevice(ctxB), unregistering ctxB->devId -- which for a never-registered/zeroed context is 0/garbage, potentially tearing down the wrong device. The PUF peripheral is genuinely a singleton so static ctx/conf is defensible, but the pointer-based API invites misuse.
Recommendation: Either document the singleton restriction explicitly in the public header, or guard Unregister against tearing down a device for a context that was never the registered one (e.g. match a stored devId/pointer before unregistering).
Referenced code: wolfcrypt/src/hwpuf.c:41; wolfcrypt/src/port/nxp/hwpuf_port.c:56-57,80 (8 lines)
Public key-code-size macro hardcoded, decoupled from NXP SDK formula
File: wolfssl/wolfcrypt/hwpuf.h:37-38; wolfcrypt/src/port/nxp/hwpuf_port.c:175,205
Function: HWPUF_KEY_SIZE_TO_KEY_CODE_SIZE / nxp_hwpuf_GenerateKey / nxp_hwpuf_GetKey
Severity: Low
Category: question
The public macro HWPUF_KEY_SIZE_TO_KEY_CODE_SIZE(keysz) always expands to the literal 52 (ignoring its argument), and callers size their key-code buffers from it. The port, however, validates buffer sizes against the NXP SDK macro PUF_GET_KEY_CODE_SIZE_FOR_KEY_SIZE(keySz) (kcSz != keyCodeSz -> BAD_FUNC_ARG). If a future NXP SDK changes its key-code size formula, the public macro and the port's validation will silently diverge and every GenerateKey/GetKey call will fail with BAD_FUNC_ARG. Deriving the public macro from the SDK macro (or adding a compile-time assertion that they agree) would make the coupling explicit.
Recommendation: Define HWPUF_KEY_SIZE_TO_KEY_CODE_SIZE in terms of PUF_GET_KEY_CODE_SIZE_FOR_KEY_SIZE where possible, or add a static assert that the two agree for 16/24/32-byte keys so they cannot drift apart.
Referenced code: wolfssl/wolfcrypt/hwpuf.h:37-38; wolfcrypt/src/port/nxp/hwpuf_port.c:175,205 (5 lines)
Function-like macros missing parentheses around parameter
File: wolfssl/wolfcrypt/port/nxp/hwpuf_port.h:32-33; wolfssl/wolfcrypt/hwpuf.h:38
Function: HWPUF_KEY_SIZE_IS_VALID / HWPUF_KEY_SIZE_TO_KEY_CODE_SIZE
Severity: Info
Category: style
HWPUF_KEY_SIZE_IS_VALID(keysz) expands keysz without wrapping it in parentheses ((keysz == 16 || keysz == 24 || keysz == 32)); a caller passing an expression rather than a plain variable could hit operator-precedence surprises. (Current callers pass simple vars so it is harmless today.) Separately, HWPUF_KEY_SIZE_TO_KEY_CODE_SIZE(keysz) discards its argument entirely (always 52), which is intentional but worth a comment so readers do not assume it scales with key size.
Recommendation: Parenthesize the macro parameter in HWPUF_KEY_SIZE_IS_VALID: ((keysz) == 16 || (keysz) == 24 || (keysz) == 32); keep a comment noting the key-code size is fixed at 52.
Referenced code: wolfssl/wolfcrypt/port/nxp/hwpuf_port.h:32-33; wolfssl/wolfcrypt/hwpuf.h:38-41 (4 lines)
Review generated by Skoll
- reworked activationCode and added to api - updated test.c - improved error checking - reworked device id usage - more
364b7df to
2a1005c
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10584
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfssl-src, wolfssl-bugs
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
daa7f0a to
78667da
Compare
This adds hardware puf support to wolfCrypt...
An effort has been made to be as compatible with the sw puf (WOLFSSL_PUF) implementation as possible, but the two fundamentally diverge in certain respects.
Not all features of the hw puf are supported yet. Specifically, this initial implementation supports...
Note: this pr goes with wolfSSL/wolfBoot#787
Testing
Tested with new test functionality added to wolfcrypt/test/test.c
Checklist