bfdd: fix stack buffer overflow in bp_raw_sbfd_red_send()

[guoguojia2021]

bp_raw_sbfd_red_send() constructs a raw SRv6-encapsulated BFD packet in a stack-allocated 1024-byte buffer (sendbuf[BUF_SIZ]). The Segment Routing Header (SRH) grows by 16 bytes per segment, and seg_num is a uint8_t parameter (max 255) supplied from the caller without prior validation.

Before this fix, a crafted or misconfigured seg_num value (e.g. 64) would cause the SRH alone to consume ~1064 bytes, exceeding the buffer. The subsequent memcpy() of the BFD payload then writes past the end of sendbuf, corrupting the stack frame — potentially the saved frame pointer, return address, or adjacent local variables.

[greptile-apps]

Greptile Summary

This PR fixes a stack buffer overflow in bp_raw_sbfd_red_send() by adding two input validation guards to prevent out-of-bounds writes into the 1024-byte stack-allocated sendbuf.

• Adds an early check rejecting seg_num > SRV6_MAX_SEGS (8), which bounds the maximum SRH header size to ~120 bytes and prevents the segment list loop from overrunning the buffer during packet construction.
• Adds a pre-memcpy check verifying total_len + datalen fits within sendbuf before copying the BFD payload, providing defense-in-depth for the final write.

Confidence Score: 4/5

Safe to merge — the two new guards close the described stack overflow and are correct for current code. The remaining notes are robustness observations that do not affect correctness today.

Both guards are logically sound. The seg_num check caps the accumulated header size at ~208 bytes (well below the 1024-byte buffer), and the pre-memcpy check prevents datalen from overrunning the remainder. The only edge cases are theoretical: the (int)datalen cast would misfire only for datalen > INT_MAX, which BFD packets never reach, and intermediate header writes rely implicitly on the first guard rather than carrying their own bounds assertion.

bfdd/bfd_packet.c — specifically the area between the seg_num guard and the second memcpy guard, where intermediate header writes have no independent bounds check.

Important Files Changed

Filename

Overview

bfdd/bfd_packet.c

Adds two bounds-checks to bp_raw_sbfd_red_send(): a seg_num guard against SRV6_MAX_SEGS (= 8) at function entry, and a total_len + datalen guard before the final memcpy. Both guards are correct and address the described stack overflow. Minor: the second guard uses (int)datalen which could theoretically wrap for abnormally large datalen values, and intermediate header writes are not independently guarded (safe only because seg_num <= 8 caps header size well below BUF_SIZ).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[bp_raw_sbfd_red_send called] --> B{seg_num > SRV6_MAX_SEGS?}
    B -- Yes --> C[zlog_err + return -1 NEW GUARD]
    B -- No --> D[memset sendbuf 0]
    D --> E{seg_num > 0?}
    E -- Yes --> F[Write outer IPv6 header total_len += 40]
    E -- No --> G
    F --> G{seg_num > 1?}
    G -- Yes --> H[Write SRH routing header total_len += 8 + 16 x seg_num-1]
    G -- No --> I
    H --> I{family == AF_INET6?}
    I -- Yes --> J[Write inner IPv6 + UDP total_len += 48]
    I -- No --> K[Write inner IPv4 + UDP total_len += 28]
    J --> L{total_len + datalen > sizeof sendbuf?}
    K --> L
    L -- Yes --> M[zlog_err + return -1 NEW GUARD]
    L -- No --> N[memcpy BFD payload]
    N --> O[sendmsg]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[bp_raw_sbfd_red_send called] --> B{seg_num > SRV6_MAX_SEGS?}
    B -- Yes --> C[zlog_err + return -1 NEW GUARD]
    B -- No --> D[memset sendbuf 0]
    D --> E{seg_num > 0?}
    E -- Yes --> F[Write outer IPv6 header total_len += 40]
    E -- No --> G
    F --> G{seg_num > 1?}
    G -- Yes --> H[Write SRH routing header total_len += 8 + 16 x seg_num-1]
    G -- No --> I
    H --> I{family == AF_INET6?}
    I -- Yes --> J[Write inner IPv6 + UDP total_len += 48]
    I -- No --> K[Write inner IPv4 + UDP total_len += 28]
    J --> L{total_len + datalen > sizeof sendbuf?}
    K --> L
    L -- Yes --> M[zlog_err + return -1 NEW GUARD]
    L -- No --> N[memcpy BFD payload]
    N --> O[sendmsg]

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
bfdd/bfd_packet.c:2948-2952
**Second guard does not cover intermediate header writes**

The check at line 2948 only guards the final `memcpy(payload, data, datalen)`. The earlier writes — `bp_sbfd_encap_srh_ip6h_red`, `bp_sbfd_encap_srh_rth_red`, `bp_sbfd_encap_inner_ip6h/iph`, and `bp_sbfd_encap_udp6/4` — all write into `sendbuf` before this check is ever reached. Those writes are safe today because `seg_num <= SRV6_MAX_SEGS (8)` caps the maximum accumulated header size at roughly 208 bytes (well below 1024), but if `SRV6_MAX_SEGS` is increased or new headers are inserted without updating the constant, intermediate overflows would silently bypass this guard entirely. A lightweight intermediate check (e.g., `assert(total_len <= (int)sizeof(sendbuf))`) after the SRH block would make the invariant explicit and catch regressions early.

### Issue 2 of 2
bfdd/bfd_packet.c:2948-2952
The `(int)datalen` cast in the overflow check can produce a negative value when `datalen > INT_MAX`, which would make the comparison vacuously false and allow a write past the buffer. Use unsigned arithmetic or check `datalen` independently to make the guard unconditionally correct.

```suggestion
	if (datalen > sizeof(sendbuf) || total_len > (int)sizeof(sendbuf) - (int)datalen) {
		zlog_err("%s: packet too large (%d + %zu > %zu)",
			 __func__, total_len, datalen, sizeof(sendbuf));
		return -1;
	}
```

_{Reviews (1): Last reviewed commit: "bfdd: fix stack buffer overflow in bp_ra..." | Re-trigger Greptile}

[greptile-apps]

Second guard does not cover intermediate header writes

The check at line 2948 only guards the final memcpy(payload, data, datalen). The earlier writes — bp_sbfd_encap_srh_ip6h_red, bp_sbfd_encap_srh_rth_red, bp_sbfd_encap_inner_ip6h/iph, and bp_sbfd_encap_udp6/4 — all write into sendbuf before this check is ever reached. Those writes are safe today because seg_num <= SRV6_MAX_SEGS (8) caps the maximum accumulated header size at roughly 208 bytes (well below 1024), but if SRV6_MAX_SEGS is increased or new headers are inserted without updating the constant, intermediate overflows would silently bypass this guard entirely. A lightweight intermediate check (e.g., assert(total_len <= (int)sizeof(sendbuf))) after the SRH block would make the invariant explicit and catch regressions early.

Prompt To Fix With AI

This is a comment left during a code review.
Path: bfdd/bfd_packet.c
Line: 2948-2952

Comment:
**Second guard does not cover intermediate header writes**

The check at line 2948 only guards the final `memcpy(payload, data, datalen)`. The earlier writes — `bp_sbfd_encap_srh_ip6h_red`, `bp_sbfd_encap_srh_rth_red`, `bp_sbfd_encap_inner_ip6h/iph`, and `bp_sbfd_encap_udp6/4` — all write into `sendbuf` before this check is ever reached. Those writes are safe today because `seg_num <= SRV6_MAX_SEGS (8)` caps the maximum accumulated header size at roughly 208 bytes (well below 1024), but if `SRV6_MAX_SEGS` is increased or new headers are inserted without updating the constant, intermediate overflows would silently bypass this guard entirely. A lightweight intermediate check (e.g., `assert(total_len <= (int)sizeof(sendbuf))`) after the SRH block would make the invariant explicit and catch regressions early.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

The (int)datalen cast in the overflow check can produce a negative value when datalen > INT_MAX, which would make the comparison vacuously false and allow a write past the buffer. Use unsigned arithmetic or check datalen independently to make the guard unconditionally correct.

Suggested change

	if (total_len + (int)datalen > (int)sizeof(sendbuf)) {
	zlog_err("%s: packet too large (%d + %zu > %zu)",
	__func__, total_len, datalen, sizeof(sendbuf));
	return -1;
	}
	if (datalen > sizeof(sendbuf) || total_len > (int)sizeof(sendbuf) - (int)datalen) {
	zlog_err("%s: packet too large (%d + %zu > %zu)",
	__func__, total_len, datalen, sizeof(sendbuf));
	return -1;
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: bfdd/bfd_packet.c
Line: 2948-2952

Comment:
The `(int)datalen` cast in the overflow check can produce a negative value when `datalen > INT_MAX`, which would make the comparison vacuously false and allow a write past the buffer. Use unsigned arithmetic or check `datalen` independently to make the guard unconditionally correct.

```suggestion
	if (datalen > sizeof(sendbuf) || total_len > (int)sizeof(sendbuf) - (int)datalen) {
		zlog_err("%s: packet too large (%d + %zu > %zu)",
			 __func__, total_len, datalen, sizeof(sendbuf));
		return -1;
	}
```

How can I resolve this? If you propose a fix, please make it concise.