Conversation
9c0208f to
eae465e
Compare
b186ecc to
f529484
Compare
src/fwtpm/fwtpm_command.c
Outdated
| FwFlushAllSessions(ctx); | ||
| } | ||
|
|
||
| FWTPM_NV_Save(ctx); |
There was a problem hiding this comment.
Return value is discarded here, this functions will return success regardless of FWTPM_NV_Save result. Review error handling in this function.
danielinux
left a comment
There was a problem hiding this comment.
FWTPM_NV_Save() return value is still discarded in several places, unsure if the error propagation has been addressed when FWTPM_NV_Save() fails
|
checking the fixes more closely |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 62 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
src/tpm2_crypto.c:1
- TPM2_KDFa_ex omits the required label terminator (single 0x00 byte) when label is NULL. Per the TPM 2.0 KDFa definition, the input includes
label || 0x00even for an empty label; skipping it changes derived keys depending on whether callers pass NULL vs "". Consider treating NULL as an empty label and always hashing exactly one 0x00 byte for the label separator.
src/tpm2_crypto.c:1 - TPM2_KDFe_ex has the same issue as KDFa: when label is NULL, the function does not hash the mandatory
0x00label separator. This makes KDFe outputs depend on NULL vs empty-string usage. Fix by always hashing a single zero byte when label is NULL (or by normalizing NULL to "" and hashing the terminator unconditionally).
src/tpm2_packet.c:1 - TPM2_Packet_ParseU16Buf silently truncates oversized fields and overwrites the caller’s
sizeoutput with the truncated length. This can cause higher-level code to accept attacker-controlled responses/structures that are malformed (e.g., truncated names/digests/policies) without any error signal. Consider changing this helper to return a status (e.g., TPM_RC_SIZE / BUFFER_E) whenwireSize > maxBufSz, and update call sites to fail parsing rather than clamping.
tests/unit_tests.c:1 - The updated unit test now exercises only TPM2_KDFa_ex and no longer validates the backward-compatible TPM2_KDFa wrapper. Since the wrapper remains part of the public API, add an assertion that TPM2_KDFa(...) returns the same output as TPM2_KDFa_ex(...) for the same inputs (and/or restore direct coverage of TPM2_KDFa).
src/fwtpm/fwtpm_tis_shm.c:1 - Unconditionally calling sem_unlink() on fixed global semaphore names can break a concurrently running fwTPM instance (or an instance started by another user/session on the same host), since it removes the names out from under the other process. Consider using instance-unique semaphore names (e.g., include PID/UID), or only unlink when you detect stale resources that belong to this instance (e.g., via a unique shared-memory header + ownership/nonce check).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 72 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/tpm2_swtpm.c:1
- In UART mode,
SwTpmDisconnectnever closes the file descriptor and never resetsctx->tcpCtx.fdto-1. If higher-level code expects...Disconnect()to release resources (e.g., duringTPM2_Cleanup) or to force a reconnect after an I/O error, the UART path will leak the FD and may prevent recovery. Consider closing/resetting the UART FD during final cleanup and/or on transmit/receive errors (while still allowing “keep open across commands” during normal operation).
/* tpm2_swtpm.c
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 74 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 75 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* 8N1: 8 data bits, no parity, 1 stop bit */ | ||
| tty.c_cflag = (tty.c_cflag & ~CSIZE) | CS8; | ||
| tty.c_cflag &= ~(PARENB | PARODD | CSTOPB | CRTSCTS); |
There was a problem hiding this comment.
The B230400, B460800, and B921600 baud constants are not available on all platforms/termios implementations, which can cause compile failures when WOLFTPM_SWTPM_UART is enabled. Guard these case labels with #ifdef Bxxxxxx (or provide a compatibility mapping/fallback) so the UART transport remains portable across Linux distros and macOS/BSD variants.
| /* 8N1: 8 data bits, no parity, 1 stop bit */ | |
| tty.c_cflag = (tty.c_cflag & ~CSIZE) | CS8; | |
| tty.c_cflag &= ~(PARENB | PARODD | CSTOPB | CRTSCTS); | |
| case 115200: baud = B115200; break; | |
| #ifdef B230400 | |
| case 230400: baud = B230400; break; | |
| #endif | |
| #ifdef B460800 | |
| case 460800: baud = B460800; break; | |
| #endif | |
| #ifdef B921600 | |
| case 921600: baud = B921600; break; | |
| #endif |
| # Wait for a TCP port to be listening (for servers without ready-file support) | ||
| # Uses ss to check without connecting (nc -z would consume the accept slot) | ||
| wait_for_port() { | ||
| local port="$1" timeout="${2:-500}" elapsed=0 | ||
| while [ $elapsed -lt $timeout ]; do | ||
| if ss -tln 2>/dev/null | grep -q ":${port} "; then | ||
| return 0 | ||
| fi | ||
| sleep 0.01 | ||
| elapsed=$((elapsed + 1)) | ||
| done | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
wait_for_port hard-depends on ss, which is typically unavailable on macOS and some minimal CI/container images. Since this function is now used to gate TLS tests, missing ss will cause spurious test failures. Consider adding a fallback chain (e.g., ss -> netstat -> lsof -> nc -z) or detect missing tooling and revert to a bounded sleep approach.
| echo -e "./examples/server/server -v $3 -p $port -w -g -A ./certs/tpm-ca-$1-cert.pem -R $READY_FILE" | ||
| ./examples/server/server -v $3 -p $port -w -g -A ./certs/tpm-ca-$1-cert.pem -R "$READY_FILE" >> $TPMPWD/run.out 2>&1 & | ||
| popd >> $TPMPWD/run.out 2>&1 | ||
| sleep 0.1 | ||
| if ! wait_for_ready "$READY_FILE" 500; then | ||
| echo -e "wolfSSL server failed to start for $1 $2" | ||
| rm -f "$READY_FILE" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The TLS test now assumes wolfSSL’s server example supports the -R ready-file flag. If a user’s wolfSSL tree/system install lacks that option, the server will exit immediately and the test will fail (even though the rest of wolfTPM might be fine). To avoid brittle coupling, consider probing for -R support (e.g., via ./examples/server/server --help) and falling back to wait_for_port when unsupported.
src/fwtpm/fwtpm_tis.c
Outdated
| if (len >= 4) { | ||
| val |= (UINT32)regs->reg_data[2] << 16; | ||
| val |= (UINT32)regs->reg_data[3] << 24; | ||
| } |
There was a problem hiding this comment.
When reg_len == 3, the third byte (reg_data[2]) is ignored because the code only incorporates bytes 2/3 when len >= 4. Even if most TIS accesses are 1/2/4 bytes, this is a correctness gap in the register emulation. Update the packing logic to include reg_data[2] when len >= 3 and reg_data[3] when len >= 4.
| if (len >= 4) { | |
| val |= (UINT32)regs->reg_data[2] << 16; | |
| val |= (UINT32)regs->reg_data[3] << 24; | |
| } | |
| if (len >= 3) val |= (UINT32)regs->reg_data[2] << 16; | |
| if (len >= 4) val |= (UINT32)regs->reg_data[3] << 24; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 75 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
CMakeLists.txt:1
wolftpm_wolfssl_depis an INTERFACE library. Installing it viainstall(TARGETS ...)while also specifying LIBRARY/ARCHIVE/RUNTIME destinations can be rejected or behave inconsistently across CMake versions/toolchains. To maximize compatibility, install/export the interface target separately (export-only), or omit artifact destinations for it and keep destinations only forwolftpm.
# CMakeList.txt
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wait_for_port() { | ||
| local port="$1" timeout="${2:-500}" elapsed=0 | ||
| while [ $elapsed -lt $timeout ]; do | ||
| if ss -tln 2>/dev/null | grep -q ":${port} "; then | ||
| return 0 | ||
| fi | ||
| sleep 0.01 | ||
| elapsed=$((elapsed + 1)) | ||
| done | ||
| return 1 | ||
| } | ||
|
|
||
| # Pick an available random port (returns port on stdout) | ||
| pick_available_port() { | ||
| local port attempts=0 | ||
| while [ $attempts -lt 20 ]; do | ||
| if command -v shuf > /dev/null 2>&1; then | ||
| port=$(shuf -i 10000-65000 -n 1) | ||
| else | ||
| port=$(( (RANDOM % 55000) + 10000 )) | ||
| fi | ||
| if ! nc -z localhost "$port" 2>/dev/null; then | ||
| echo "$port" | ||
| return 0 | ||
| fi | ||
| attempts=$((attempts + 1)) | ||
| done | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
wait_for_port hard-depends on ss; if ss isn’t installed, the loop will always time out even when the server is listening. Also pick_available_port uses nc -z without verifying nc exists; when nc is missing, the command returns 127 and the script will incorrectly treat the port as “available”. Add tool detection + fallback (e.g., ss → netstat, and for port checks use ss/netstat or a small Python socket bind probe when nc isn’t available).
| run_tpm_tls_client() { # Usage: run_tpm_tls_client [ecc/rsa] [tpmargs] [tlsversion] | ||
| echo -e "TLS test (TPM as client) $1 $2 $3" | ||
| generate_port | ||
| READY_FILE="/tmp/wolftpm_tls_ready_$$" | ||
| rm -f "$READY_FILE" | ||
| pushd $WOLFSSL_PATH >> $TPMPWD/run.out 2>&1 | ||
| echo -e "./examples/server/server -v $3 -p $port -w -g -A ./certs/tpm-ca-$1-cert.pem" | ||
| ./examples/server/server -v $3 -p $port -w -g -A ./certs/tpm-ca-$1-cert.pem >> $TPMPWD/run.out 2>&1 & | ||
| RESULT=$? | ||
| [ $RESULT -ne 0 ] && echo -e "tls server $1 $2 failed! $RESULT" && exit 1 | ||
| echo -e "./examples/server/server -v $3 -p $port -w -g -A ./certs/tpm-ca-$1-cert.pem -R $READY_FILE" | ||
| ./examples/server/server -v $3 -p $port -w -g -A ./certs/tpm-ca-$1-cert.pem -R "$READY_FILE" >> $TPMPWD/run.out 2>&1 & | ||
| popd >> $TPMPWD/run.out 2>&1 | ||
| sleep 0.1 | ||
| if ! wait_for_ready "$READY_FILE" 500; then | ||
| echo -e "wolfSSL server failed to start for $1 $2" | ||
| rm -f "$READY_FILE" | ||
| exit 1 | ||
| fi | ||
| rm -f "$READY_FILE" |
There was a problem hiding this comment.
The TLS server is started in the background but its PID isn’t captured/cleaned up on failure. If the -R flag isn’t supported by the wolfSSL server binary (or if it crashes before creating the ready file), this path exits and can leave a stray server process bound to the port, causing cascading failures in later tests. Capture $! and ensure it’s terminated on the timeout path; additionally consider falling back to wait_for_port if the ready file never appears (to keep compatibility with wolfSSL versions/builds that don’t support -R).
|
|
||
| /* 8N1: 8 data bits, no parity, 1 stop bit */ | ||
| tty.c_cflag = (tty.c_cflag & ~CSIZE) | CS8; | ||
| tty.c_cflag &= ~(PARENB | PARODD | CSTOPB | CRTSCTS); |
There was a problem hiding this comment.
CRTSCTS is not defined on all platforms that have termios.h (it’s common on Linux, but not guaranteed elsewhere). As written, enabling WOLFTPM_SWTPM_UART can fail to compile on platforms lacking CRTSCTS. Guard this flag (e.g., #ifdef CRTSCTS) or use the platform-specific flow-control flags where applicable.
| tty.c_cflag &= ~(PARENB | PARODD | CSTOPB | CRTSCTS); | |
| tty.c_cflag &= ~(PARENB | PARODD | CSTOPB); | |
| #ifdef CRTSCTS | |
| tty.c_cflag &= ~CRTSCTS; | |
| #endif |
Summary
TPM2_Packet_ParseU16Bufvariant for safer response parsingfwTPM Server
Core TPM 2.0 command processing in
src/fwtpm/:fwtpm_command.c— 105 command handlers with full auth, sessions, parameter encryptionfwtpm_nv.c— TLV journal NV storage (file-based default, HAL-abstracted for flash)fwtpm_io.c— Socket transport (mssim + swtpm protocol auto-detection)fwtpm_tis.c/fwtpm_tis_shm.c— TIS register interface via POSIX shared memoryfwtpm_crypto.c— Key generation, sign/verify, seed encrypt/decrypt helpersBuild:
./configure --enable-fwtpm && makeExample: wolfSSL/wolftpm-examples#1
Primary Key Derivation (TPM 2.0 Part 1 Section 26)
UART Transport (
--enable-swtpm=uart)New transport option for wolfTPM client library to communicate with embedded fwTPM over serial:
./configure --enable-swtpm=uart— uses termios serial I/O instead of TCP socketsTPM2_SWTPM_HOSTenv var selects serial device at runtimeTesting
scripts/tpm2_tools_test.sh)examples/run_examples.sh)tests/fuzz/)scripts/fwtpm_emu_test.sh)