Skip to content

nhrpd: Correct addrlen check in os_recvmsg()#21100

Merged
donaldsharp merged 1 commit intoFRRouting:masterfrom
csiltala:nhrpd-addrlen-check
Mar 12, 2026
Merged

nhrpd: Correct addrlen check in os_recvmsg()#21100
donaldsharp merged 1 commit intoFRRouting:masterfrom
csiltala:nhrpd-addrlen-check

Conversation

@csiltala
Copy link

Previously compared addrlen to the stack address of lladdr.sll_addr cast to size_t, virtually always true.

This should remain always true as sll_addr is an unsigned char array of size 8 and addr is an array of size 64 but this fixes the check and ensures enough space in addr for memcpy.

Previously compared addrlen to the stack address of lladdr.sll_addr cast to size_t, virtually always true.

This should remain always true as sll_addr is an unsigned char array of size 8 and addr is an array of size 64 but this fixes the check and ensures enough space in addr for memcpy.

Signed-off-by: Corey Siltala <csiltala@atcorp.com>
@greptile-apps
Copy link

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR fixes a clear, pre-existing bug in os_recvmsg() inside nhrpd/linux.c where the bounds check before a memcpy was comparing *addrlen (the caller's buffer size) against (size_t)lladdr.sll_addr — the memory address of the sll_addr array field cast to size_t — rather than the actual hardware address length. Because a stack pointer cast to size_t is typically a very large value, the condition was virtually always true, making the guard meaningless.

The fix replaces the comparison with the semantically correct lladdr.sll_halen <= *addrlen, which ensures the received hardware address length fits within the caller-supplied buffer before the memcpy is performed.

  • Bug fix: (size_t)lladdr.sll_addr (a pointer cast) replaced by lladdr.sll_halen (actual hardware address length) in the bounds check.
  • The caller in nhrp_packet.c always passes a 64-byte addr buffer and addrlen = sizeof(addr), so sll_halen (≤ 8 for Ethernet, 4 for IPv4 NBMA) will always satisfy the new condition under normal circumstances, preserving existing functional behaviour.
  • The fix also correctly protects against the theoretical case where sll_halen exceeds the caller's buffer, which the old code could not guard against.

Confidence Score: 5/5

  • This PR is safe to merge — it is a minimal, targeted bug fix with no logic regressions under normal operating conditions.
  • The change is a single-line fix correcting an obvious comparison bug (pointer address vs. buffer length). The new condition lladdr.sll_halen <= *addrlen is the semantically correct bounds check before the memcpy. All call sites pass a sufficiently large buffer (64 bytes), so existing behaviour is preserved while the potential buffer-overread is now properly guarded. No new code paths are introduced.
  • No files require special attention.

Important Files Changed

Filename Overview
nhrpd/linux.c Fixes a clear bug in os_recvmsg() where the bounds check before memcpy was comparing the addrlen value to the memory address of sll_addr (meaningless), replacing it with a correct comparison of sll_halen to the caller-supplied buffer size.

Sequence Diagram

sequenceDiagram
    participant Caller as nhrp_packet_recvraw
    participant RecvMsg as os_recvmsg()
    participant Kernel as recvmsg() / kernel

    Caller->>RecvMsg: addr[64], addrlen=64
    RecvMsg->>Kernel: recvmsg(nhrp_socket_fd, MSG_DONTWAIT)
    Kernel-->>RecvMsg: fills sockaddr_ll (sll_halen, sll_addr)
    Note over RecvMsg: NEW: if (sll_halen <= *addrlen)<br/>OLD (buggy): if (*addrlen <= (size_t)sll_addr)
    alt sll_halen fits in buffer (sll_halen <= addrlen)
        RecvMsg->>RecvMsg: memcpy(addr, sll_addr, sll_halen)
        RecvMsg->>RecvMsg: *addrlen = sll_halen
    else buffer too small
        RecvMsg->>RecvMsg: skip copy, addrlen unchanged
    end
    RecvMsg-->>Caller: addr[], *addrlen (e.g. 4 for IPv4)
    Caller->>Caller: switch(addrlen) → case 4: process packet
Loading

Last reviewed commit: 3dc2d46

@Jafaral
Copy link
Member

Jafaral commented Mar 11, 2026

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4

@mergify
Copy link

mergify bot commented Mar 11, 2026

backport stable/10.6 stable/10.5 stable/10.4

✅ Backports have been created

Details

@donaldsharp
Copy link
Member

LGTM

@donaldsharp donaldsharp merged commit e119526 into FRRouting:master Mar 12, 2026
23 checks passed
Jafaral added a commit that referenced this pull request Mar 12, 2026
nhrpd: Correct addrlen check in os_recvmsg() (backport #21100)
Jafaral added a commit that referenced this pull request Mar 12, 2026
nhrpd: Correct addrlen check in os_recvmsg() (backport #21100)
Jafaral added a commit that referenced this pull request Mar 12, 2026
nhrpd: Correct addrlen check in os_recvmsg() (backport #21100)
@mattiaswal mattiaswal mentioned this pull request Mar 18, 2026
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants