20260329-fenrir-fixes

[douzzer]

Fixes for F-1556 F-1557 F-1558 F-1559 F-1563 F-1560 F-1561 F-1562 F-1564 F-1565

tested with

WOLFGUARD_BRANCH=20260329-fenrir-fixes wolfssl-multi-test.sh
quantum-safe-wolfssl-all-crypto-only-noasm-linuxkm-mainline-insmod-clang-tidy
quantum-safe-wolfssl-all-crypto-only-intelasm-sp-asm-linuxkm-mainline-insmod
quantum-safe-wolfssl-all-crypto-only-intelasm-sp-asm-fips-dev-linuxkm-next-insmod
linuxkm-6.15-all-cryptonly-quantum-safe-fips-dev-intelasm-insmod-crypto-fuzzer-kmemleak
linuxkm-6.15-all-cryptonly-quantum-safe-fips-dev-intelasm-insmod-crypto-fuzzer-ksan
linuxkm-fips-v5-cert4718-insmod-wolfguard-cust-kernel-3
linuxkm-fips-dev-insmod-wolfguard-cust-kernel-3

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #11

Scan targets checked: wolfguard-compliance, wolfguard-kernel-bugs, wolfguard-kernel-src, wolfguard-user-bugs, wolfguard-user-src
Findings: 3

High (1)

Missing peer->next_peer = NULL when appending to linked list in sync_conf

File: user-src/setconf.c:89-96
Function: sync_conf
Category: Logic errors

The linked list insertion logic was changed from prepending to appending, but the new code never sets peer->next_peer = NULL. The old code explicitly set peer->next_peer = file->first_peer (which was NULL when the list was empty, and a valid pointer otherwise). The new append logic sets file->last_peer = peer but never nullifies peer->next_peer to terminate the list. If peer is allocated with malloc (rather than calloc), peer->next_peer will contain uninitialized memory, causing linked list traversal (via for_each_wgpeer) to follow a garbage pointer. Even if calloc is used, the omission is a latent bug — any future change to the allocator or struct layout could expose it. The list termination was guaranteed by the old prepend pattern but is lost in the new append pattern.

if (! file->first_peer)
	file->first_peer = peer;
if (file->last_peer)
	file->last_peer->next_peer = peer;
file->last_peer = peer;
/* peer->next_peer is never set to NULL */

Recommendation: Add peer->next_peer = NULL; before the list insertion logic, e.g.:

peer->next_peer = NULL;
if (! file->first_peer)
	file->first_peer = peer;
if (file->last_peer)
	file->last_peer->next_peer = peer;
file->last_peer = peer;

---

Medium (1)

Uninitialized ret variable returned from wg_receive_handshake_packet

File: kernel-src/receive.c:120
Function: wg_receive_handshake_packet
Category: Logic errors

The PR hoists int ret; to function scope (line ~120) without initializing it, and changes the function's final return from return 0; to return ret; (line ~229). Within the switch statement, ret is assigned in both the MESSAGE_HANDSHAKE_INITIATION and MESSAGE_HANDSHAKE_RESPONSE cases. However, the switch has no default case. If execution somehow reaches the switch without matching either case label, control falls through to the post-switch code and return ret; with ret still uninitialized — this is undefined behavior in C. While in practice the pre-switch logic likely ensures only INITIATION or RESPONSE packets reach the switch, the compiler has no way to prove this, and the original code was safe because it unconditionally returned 0. This is a regression introduced by the PR.

int ret;
...
switch (SKB_TYPE_LE32(skb)) {
case cpu_to_le32(MESSAGE_HANDSHAKE_INITIATION): {
    ...
    ret = 0;
    break;
}
case cpu_to_le32(MESSAGE_HANDSHAKE_RESPONSE): {
    ...
    ret = 0; /* or -ECANCELED */
    break;
}
} /* no default case */
...
return ret; /* potentially uninitialized */

Recommendation: Initialize ret at declaration: int ret = 0;. This preserves the original safe default while still allowing the new per-case error propagation.

---

Low (1)

TOCTOU race on last_sent_handshake restoration in failure path

File: kernel-src/send.c:26-52
Function: wg_packet_send_handshake_initiation
Category: Race conditions

The PR reads cur_last_sent_handshake = atomic64_read(&peer->last_sent_handshake) at function entry, then on the failure path restores it with atomic64_set(&peer->last_sent_handshake, cur_last_sent_handshake). Between the read and the set, a concurrent call to this function for the same peer could succeed and update last_sent_handshake to the current time. The failure path's atomic64_set would then overwrite the successful call's timestamp with the stale value, effectively resetting the REKEY_TIMEOUT rate limit. This could allow more frequent handshake initiation attempts than intended. The individual atomic operations are each atomic, but the read-then-restore sequence is not — atomic64_cmpxchg would be needed for a correct restore. The practical impact is limited because handshake initiation is internally triggered and the race window is narrow, but under persistent handshake failure conditions the rate limit would be continuously bypassed.

u64 cur_last_sent_handshake = atomic64_read(&peer->last_sent_handshake);

if (!wg_birthdate_has_expired(cur_last_sent_handshake,
                              REKEY_TIMEOUT))
    return 0;
...
else {
    atomic64_set(&peer->last_sent_handshake, cur_last_sent_handshake);
    ret = -ECANCELED;
}

Recommendation: Use atomic64_cmpxchg(&peer->last_sent_handshake, cur_last_sent_handshake, cur_last_sent_handshake) to only restore if no concurrent update occurred, or simply remove the restoration and let the rate limit apply even on failure (matching the pre-PR behavior).

---

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

[douzzer]

High (1)

Missing peer->next_peer = NULL when appending to linked list in sync_conf

False positive. calloc() is local to the code at issue, and the allocated struct wgpeer contains additional pointers that are implicitly initialized by the calloc() in the incumbent code. This project does not support targets where NULL != 0.

[douzzer]

Medium (1)

Uninitialized ret variable returned from wg_receive_handshake_packet

This is a deliberate decision. ret is always set when peer is set in the switch block, and if peer is unset after the switch block, the function returns immediately with -EBADMSG. We use various static analyzers on these files and we want a may-be-used-uninitialized warning for ret if a future change disrupts the architecture of the function.

[douzzer]

Low (1)

TOCTOU race on last_sent_handshake restoration in failure path

Fixed.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #11

Scan targets checked: wolfguard-compliance, wolfguard-kernel-bugs, wolfguard-kernel-src, wolfguard-user-bugs, wolfguard-user-src
Findings: 1

Medium (1)

Uninitialized ret variable on default switch path in wg_receive_handshake_packet

File: kernel-src/receive.c:120
Function: wg_receive_handshake_packet
Category: Logic errors

The PR introduces int ret; (uninitialized) at line 120 and changes the function's return from a hardcoded return 0 to return ret at line 229. The switch statement has cases for MESSAGE_HANDSHAKE_INITIATION and MESSAGE_HANDSHAKE_RESPONSE, both of which properly assign ret. However, there is no default: case. If execution enters the switch without matching either case, ret remains uninitialized when the function reaches return ret. While this path is likely unreachable in practice (the caller presumably validates message types), using an uninitialized variable is undefined behavior in C. The old code was safe because it unconditionally returned 0. This defect was introduced by the PR's change from return 0 to return ret.

int ret;
...
switch (SKB_TYPE_LE32(skb)) {
case cpu_to_le32(MESSAGE_HANDSHAKE_INITIATION):
    ...
    ret = 0;
    break;
case cpu_to_le32(MESSAGE_HANDSHAKE_RESPONSE):
    ...
    ret = 0; // or ret = -ECANCELED
    break;
}
// no default case
...
return ret;  // uninitialized if no case matched

Recommendation: Initialize ret at declaration: int ret = 0;. This preserves the original behavior on any unexpected path and silences compiler/static-analyzer warnings about uninitialized use.

---

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

[douzzer]

Medium (1)

Uninitialized ret variable on default switch path in wg_receive_handshake_packet

not-a-bug. Explained above.