-
Notifications
You must be signed in to change notification settings - Fork 8
Add 'unit-run' target, coverage via gcov. Fixed a bunch of fenrir reports. #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
a27998a
f999b6c
551bcea
3eaf694
b6d7d31
94b14b0
42cd2d8
e34d843
112b95d
54a4249
1e717da
6553cdf
a62cf8e
dbef108
b2df06a
f8c93dd
ba7c4ce
503f7e6
d6059f8
8b23bed
fe5fd99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -102,7 +102,18 @@ endif | |||||
| CFLAGS += $(DEBUG_FLAGS) $(SANITIZE_FLAGS) | ||||||
| LDFLAGS += $(SANITIZE_FLAGS) | ||||||
|
|
||||||
| .PHONY: all clean psa-objects | ||||||
| # gcov coverage (make cov): instrument the library and the unit tests, run | ||||||
| # the tests, and emit an HTML report of which lines of src/*.c they cover. | ||||||
| # The unit tests link the instrumented shared library, so the runtime .gcda | ||||||
| # files land next to the PIC objects in build/obj.pic. | ||||||
| COV_DIR := build/coverage | ||||||
| OPEN_CMD := $(shell command -v open >/dev/null 2>&1 && echo open || echo xdg-open) | ||||||
| ifeq ($(COV),1) | ||||||
| CFLAGS += --coverage | ||||||
| LDFLAGS += --coverage | ||||||
| endif | ||||||
|
|
||||||
| .PHONY: all clean psa-objects unit-run run-tests cov covclean | ||||||
|
|
||||||
| all: $(LIBNAME) $(SHLIBNAME) | ||||||
|
|
||||||
|
|
@@ -135,6 +146,109 @@ $(OBJDIR_PIC)/wolfcrypt_%.o: $(WOLFSSL_PATH)/wolfcrypt/src/%.c | |||||
| @mkdir -p $(OBJDIR_PIC) | ||||||
| $(CC) $(CPPFLAGS) $(DEPFLAGS) $(CFLAGS) -fPIC -c $< -o $@ | ||||||
|
|
||||||
| # The unit tests, in the order CI runs them (test-psa-api.yml). The servers | ||||||
| # (psa_tls_client, psa_tls_server), the benchmark, and psa_crypto_init_test | ||||||
| # (driven separately) are not unit tests and are not listed. | ||||||
| UNIT_TESTS := psa_api_test \ | ||||||
| psa_aead_multipart_test \ | ||||||
| psa_copy_key_narrowing_test \ | ||||||
| psa_ecc_bit_inference_test \ | ||||||
| psa_des3_stack_scrub_test \ | ||||||
| psa_ecc_curve_id_test \ | ||||||
| psa_random_size_test \ | ||||||
| psa_rsa_pss_interop_test \ | ||||||
| psa_mldsa_test \ | ||||||
| psa_mlkem_test \ | ||||||
| psa_xof_test \ | ||||||
| psa_key_wrap_test \ | ||||||
| psa_sign_context_test \ | ||||||
| psa_lms_xmss_verify_test \ | ||||||
| psa_ascon_xchacha_test \ | ||||||
| psa_sp800_108_test \ | ||||||
| psa_14_misc_test \ | ||||||
| psa_xof_input_wrap_test \ | ||||||
| psa_pbkdf2_cmac_test \ | ||||||
| psa_kdf_input_key_test \ | ||||||
| psa_ecc_verify_curve_test \ | ||||||
| psa_ecc_ecdh_curve_test \ | ||||||
| psa_xof_output_wrap_test \ | ||||||
| psa_kdf_length_check_test \ | ||||||
| psa_kdf_expand_context_test \ | ||||||
| psa_kdf_repeat_step_test \ | ||||||
| psa_kdf_psk_to_ms_size_test \ | ||||||
| psa_mldsa_det_sign_test \ | ||||||
| psa_mldsa_any_hash_test \ | ||||||
| psa_ecc_curve_caps_test \ | ||||||
| psa_xof_no_backend_test \ | ||||||
| psa_ecc_sig_len_test \ | ||||||
| psa_xof_set_context_test \ | ||||||
| psa_cipher_inplace_test \ | ||||||
| psa_cipher_overlap_test \ | ||||||
| psa_des3_pkcs7_test \ | ||||||
| psa_eddsa_mont_export_test \ | ||||||
| psa_eddsa_mont_gen_test \ | ||||||
| psa_pure_eddsa_context_test \ | ||||||
| psa_sign_hash_eddsa_test \ | ||||||
| psa_key_infer_bits_test \ | ||||||
| psa_cipher_oneshot_len_test \ | ||||||
| psa_pqc_export_seed_test \ | ||||||
| psa_key_declared_bits_test \ | ||||||
| psa_import_key_probe_test \ | ||||||
| psa_store_commit_test \ | ||||||
| psa_store_read_open_test \ | ||||||
| psa_store_dir_validation_test \ | ||||||
| psa_devid_cryptocb_test \ | ||||||
| psa_kdf_zeroize_output_test \ | ||||||
| psa_import_zero_length_test \ | ||||||
| psa_copy_key_cross_lifetime_test | ||||||
|
|
||||||
| # Run the unit test loop from the repo root (psa_rsa_pss_interop_test reads | ||||||
| # its certificate relative to the root). Assumes the tests are already built. | ||||||
| run-tests: | ||||||
| @for t in $(UNIT_TESTS); do \ | ||||||
| echo "=== $$t ==="; \ | ||||||
| rm -rf test/.store; \ | ||||||
| ./test/$$t || exit 1; \ | ||||||
| done | ||||||
|
|
||||||
| # Build the library and the unit tests, then run every unit test. | ||||||
| unit-run: all | ||||||
| @$(MAKE) -C test $(UNIT_TESTS) | ||||||
| @$(MAKE) run-tests | ||||||
|
|
||||||
| # Build everything with gcov instrumentation, run the unit tests, and emit an | ||||||
| # HTML coverage report for src/*.c (the bundled wolfCrypt sources are | ||||||
| # excluded by the -f filter). The instrumented objects are dropped once the | ||||||
| # report exists: a later non-coverage build would otherwise reuse them and | ||||||
| # fail to link (undefined __gcov_init). The report itself is kept. | ||||||
| cov: | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make cov strands the tree when a test fails. run-tests does ./test/$$t || exit 1, so the first failure aborts the recipe before gcovr runs and before the instrumented objects are removed. build/obj and build/obj.pic keep their --coverage objects, the next plain make sees them newer than their sources and links them without --coverage, and you get undefined _gcov* symbols. make covclean does not help - it only removes .gcda/.gcno. The comment at 221-223 names this hazard but nothing guards against it. Could we record the failure, still generate the report, drop the instrumented objects, then re-raise at the end? Three smaller ones while you are in here: gcovr has no preflight and is documented nowhere in the repo, so a missing one costs a full clean rebuild plus 52 binaries before it fails; --gcov-ignore-errors=all can hide a broken run and is worth narrowing; and the OPEN_CMD macOS branch at line 110 is unreachable because the guard only fires on X11/Wayland. test/Makefile clean also leaves behind the gcov artifacts it created under test/psa_server.
Suggested change
|
||||||
| @$(MAKE) clean | ||||||
| @$(MAKE) -C test clean | ||||||
| @$(MAKE) all COV=1 | ||||||
| @$(MAKE) -C test $(UNIT_TESTS) COV=1 | ||||||
| @$(MAKE) run-tests | ||||||
| @mkdir -p $(COV_DIR) | ||||||
| @echo "[COV] gcovr html" | ||||||
| @gcovr -r . -f '^src/.*\.c$$' \ | ||||||
| --gcov-ignore-errors=all \ | ||||||
| --html-medium-threshold 60 \ | ||||||
| --html-high-threshold 80 \ | ||||||
| --html-details -o $(COV_DIR)/index.html | ||||||
| @echo "[COV] report: $(COV_DIR)/index.html" | ||||||
| @echo "[COV] dropping instrumented objects" | ||||||
| @rm -rf $(OBJDIR) $(OBJDIR_PIC) $(LIBNAME) $(SHLIBNAME) | ||||||
| @$(MAKE) -C test clean | ||||||
| @if [ -n "$$DISPLAY" ] || [ -n "$$WAYLAND_DISPLAY" ]; then \ | ||||||
| $(OPEN_CMD) $(COV_DIR)/index.html || true; \ | ||||||
| fi | ||||||
|
|
||||||
| # Remove gcov artifacts and the coverage report. | ||||||
| covclean: | ||||||
| rm -f $(OBJDIR)/*.gcda $(OBJDIR)/*.gcno \ | ||||||
| $(OBJDIR_PIC)/*.gcda $(OBJDIR_PIC)/*.gcno | ||||||
| rm -f test/psa_server/*.gcda test/psa_server/*.gcno | ||||||
| rm -rf $(COV_DIR) | ||||||
|
|
||||||
| clean: | ||||||
| rm -rf $(BUILD_DIR) $(LIBNAME) $(SHLIBNAME) | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -31,5 +31,14 @@ | |||||
| #define SINGLE_THREADED | ||||||
| #define WOLFSSL_PSA_ENGINE | ||||||
| #define NO_DSA | ||||||
| /* Constant-time AES backend (F-13878): the default software AES uses | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These comment blocks say "psa_aead.c fails the build if neither it nor WOLFSSL_AESNI is selected", but the guard tests only !defined(WC_AES_BITSLICED) - a WOLFSSL_AESNI build without it still fails. The same wording is in build-test/user_settings.h, zephyr/user_settings_example.h, zephyr/tests/psa_tls_coexist/user_settings.h and its file header, and prj.conf. Someone will read it, set WOLFSSL_AESNI, and hit the same #error. Two other things in the same text: the default path is not unmitigated - wolfCrypt already prefetches every table cache line before each block unless WC_NO_CACHE_RESISTANT is set - and WC_AES_BITSLICED is not the only constant-time option; WOLFSSL_AES_TOUCH_LINES keeps Aes at 416 bytes instead of 123,296. Separately, these are long for what they are. A 12-line theory-of-operation block above the #error and a 4-line justification above a single #define in four settings files is more than a config header should carry - one line naming the constraint, and the rest in the commit message. Same for zephyr/README.md:76-97, which still documents the config this PR makes unbuildable and still says HAVE_HASHDRBG is the only hard requirement.
Suggested change
|
||||||
| * secret-indexed T-table loads, a cache-timing channel. WC_AES_BITSLICED is | ||||||
| * the portable consttime core; psa_aead.c fails the build if neither it nor | ||||||
| * WOLFSSL_AESNI is selected. */ | ||||||
| #define WC_AES_BITSLICED | ||||||
| /* psa_sign_hash()/psa_verify_hash() must accept an all-zero digest (PSA | ||||||
| * treats the hash as opaque bytes); wolfCrypt rejects it by default, so opt | ||||||
| * out. psa_ecc.c #errors when HAVE_ECC is on and this is undefined. */ | ||||||
| #define WC_ALLOW_ECC_ZERO_HASH | ||||||
|
|
||||||
| #endif /* WOLFSSL_USER_SETTINGS_H */ | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,6 +25,21 @@ | |||||||||||||||||
|
|
||||||||||||||||||
| #include <wolfssl/wolfcrypt/settings.h> | ||||||||||||||||||
|
|
||||||||||||||||||
| /* The AEAD/CMAC/KDF paths process secret-derived AES keys. Require the | ||||||||||||||||||
| * constant-time software backend (WC_AES_BITSLICED): the default software | ||||||||||||||||||
| * fallback uses secret-indexed T-table loads (a cache-timing channel), and | ||||||||||||||||||
| * WOLFSSL_AESNI alone is not sufficient because wolfCrypt falls back to | ||||||||||||||||||
| * AesSetKey_C() (the same non-constant-time T-table path) when runtime | ||||||||||||||||||
| * AES-NI is unavailable. WC_AES_BITSLICED is the consttime fallback that | ||||||||||||||||||
| * covers both the no-AES-NI and the AES-NI-unavailable cases. Fail the build | ||||||||||||||||||
| * if it is not selected, but only when the PSA engine and an AES path are | ||||||||||||||||||
| * actually active: this file is otherwise inert (no PSA engine) or its AES | ||||||||||||||||||
| * code is excluded (NO_AES), and compiling this inactive source must not | ||||||||||||||||||
| * fail. */ | ||||||||||||||||||
| #if defined(WOLFSSL_PSA_ENGINE) && !defined(NO_AES) && !defined(WC_AES_BITSLICED) | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This #error locks out every hardware AES backend. Both the T-table core and the bitsliced core live inside #ifdef NEED_AES_TABLES in wolfcrypt/src/aes.c, which WOLFSSL_ARMASM, WOLFSSL_ESP32_CRYPT, FREESCALE_LTC, WOLFSSL_SILABS_SE_ACCEL, WOLFSSL_PSOC6_CRYPTO, WOLFSSL_AFALG, WOLFSSL_DEVCRYPTO_AES, WOLFSSL_RISCV_ASM and WOLF_CRYPTO_CB_ONLY_AES never define. Those builds have no secret-indexed T-table at all, so there is no channel here to close, but they cannot build without defining WC_AES_BITSLICED. Defining it there costs them for nothing: bs_key in aes.h is gated on WC_AES_BITSLICED alone, not on NEED_AES_TABLES. Under WOLFSSL_ARMASM sizeof(Aes) goes 944 -> 123,824 while aes.c emits zero bitsliced functions. Can we make the guard accept any of the acceptable answers - a hardware backend, WOLFSSL_AES_TOUCH_LINES (constant cache-line footprint, Aes stays 416 bytes), or WC_AES_BITSLICED - and add an opt-out macro? On a cacheless M0/M3/M4 there is no cache-timing channel in the first place.
Suggested change
|
||||||||||||||||||
| #error "wolfPSA needs the consttime AES backend (WC_AES_BITSLICED); WOLFSSL_AESNI alone leaves a non-constant-time runtime fallback" | ||||||||||||||||||
| #endif | ||||||||||||||||||
|
|
||||||||||||||||||
| #if defined(WOLFSSL_PSA_ENGINE) | ||||||||||||||||||
|
|
||||||||||||||||||
| #include <psa/crypto.h> | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,23 @@ | |
|
|
||
| #include <wolfssl/wolfcrypt/settings.h> | ||
|
|
||
| /* psa_sign_hash()/psa_verify_hash() must accept an all-zero digest: PSA | ||
| * treats the hash argument as opaque bytes and ECDSA over e = 0 is | ||
| * well-defined. wolfCrypt rejects an all-zero digest by default (a guard | ||
| * against uninitialized buffers), which would surface as | ||
| * PSA_ERROR_INVALID_ARGUMENT for input the spec requires us to accept. | ||
| * WC_ALLOW_ECC_ZERO_HASH opts out of that rejection. | ||
| * | ||
| * The requirement is on the PSA ECDSA path itself, not on how the wolfCrypt | ||
| * config is supplied: any build that compiles the PSA ECC path (PSA engine + | ||
| * HAVE_ECC) must define WC_ALLOW_ECC_ZERO_HASH, whether the config comes from | ||
| * a WOLFSSL_SETTINGS_FILE, a WOLFSSL_USER_SETTINGS file, or the configure | ||
| * system. Builds without the PSA ECC path are unaffected. */ | ||
| #if defined(WOLFSSL_PSA_ENGINE) && defined(HAVE_ECC) \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both new guards enforce a project-wide policy from a single translation unit. WC_ALLOW_ECC_ZERO_HASH is consumed only when ecc.c is compiled, and psa_ecc.c never reads it - so in a split build against a prebuilt libwolfssl (which is what test/Makefile's rebuild-wolfssl-psa produces) defining it in wolfPSA's own compile satisfies the #error without changing ecc.c at all, and psa_sign_hash on an all-zero digest still returns ECC_BAD_ARG_E. The guard then reports "configured correctly" for a build that is not. The AES guard has the same shape: psa_cipher.c, psa_key_derivation.c, psa_key_wrap.c and psa_mac.c all use wc_Aes*/wc_Cmac* and carry no guard. It works today only because the Makefile and zephyr/CMakeLists.txt both glob src/*.c. Could both move into a shared header included by every TU after settings.h, and could the ECC one either key off a symbol wolfCrypt derives from settings.h or be replaced by handling the all-zero digest in psa_ecc.c directly? |
||
| && !defined(WC_ALLOW_ECC_ZERO_HASH) | ||
| #error "wolfPSA needs WC_ALLOW_ECC_ZERO_HASH (psa_sign_hash/psa_verify_hash must accept an all-zero digest)" | ||
| #endif | ||
|
|
||
| #if defined(WOLFSSL_PSA_ENGINE) && defined(HAVE_ECC) | ||
|
|
||
| #include <psa/crypto.h> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting the aes-ecb lane removes a configuration we used to support rather than fixing it. wolfCrypt hard-errors on WC_AES_BITSLICED without HAVE_AES_ECB, and this PR requires WC_AES_BITSLICED whenever AES is present, so "AES on, HAVE_AES_ECB off" is now impossible either way round.
If we want it back, HAVE_AES_ECB comes from the BASELINE list in build-test/build-variant.sh:59, so WC_AES_BITSLICED needs to move into that list for the -FLAG modifier to strip it. Otherwise let's state in user_settings.h and CHANGELOG.md that HAVE_AES_ECB is now a hard requirement. Worth adding a NO_AES lane too - the guard's own !defined(NO_AES) escape hatch is currently untested.