bgpd: add initial BGP-MUP SAFI support (draft-ietf-bess-mup-safi)#22311
bgpd: add initial BGP-MUP SAFI support (draft-ietf-bess-mup-safi)#22311higebu wants to merge 9 commits into
Conversation
Greptile SummaryThis PR adds control-plane support for BGP-MUP SAFI (draft-ietf-bess-mup-safi), enabling FRR to act as a route reflector for the SRv6 Mobile User Plane architecture. The change is well-scoped: route origination and FIB installation are explicitly deferred.
Confidence Score: 3/5The overall implementation is structurally sound and the reflection path is well-exercised by the topotest, but the T2ST TEID masking gap means a non-conformant peer can insert routes that violate a MUST constraint from the draft. The T2ST decoder stores raw wire bits—including unspecified trailing padding bits—into the route key without masking. When a peer sets those bits non-zero, a TEID that is semantically zero passes the TEID≠0 validity check, admitting a route the draft explicitly requires to be treated as malformed. All other parsing paths (ISD, DSD, T1ST) look correct, and the encoder/decoder round-trip is consistent for well-behaved peers. bgpd/bgp_mup.c — specifically bgp_mup_process_t2st_route (TEID masking and the T2ST encoder teid_octets bound) Important Files Changed
Sequence DiagramsequenceDiagram
participant Peer as BGP Peer
participant Pkt as bgp_packet.c
participant Attr as bgp_attr.c
participant Mup as bgp_mup.c
participant RIB as BGP RIB
Peer->>Pkt: UPDATE (MP_REACH SAFI_MUP)
Pkt->>Attr: bgp_mp_reach_parse()
Attr->>Mup: bgp_nlri_parse_mup()
Note over Mup: Decode arch_type / route_type / length<br/>Validate per-type bounds and constraints
Mup->>RIB: bgp_update() / bgp_withdraw()
RIB->>Attr: bgp_packet_mpattr_start() [nexthop]
RIB->>Attr: bgp_packet_mpattr_prefix()
Attr->>Mup: bgp_mup_encode_prefix()
Mup-->>Peer: Reflected UPDATE
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
bgpd/bgp_mup.c:393-410
**T2ST partial-TEID padding bits not masked — TEID=0 check can be bypassed**
When `teid_bits % 8 != 0` (e.g., `ea_len = 35` for IPv4 means 3 significant TEID bits and 5 padding bits in the last byte), `teid_be` is zero-initialised and then `memcpy` fills the low-address bytes on little-endian systems. After `ntohl`, the significant TEID bits land at the MSB, but the 5 padding bits also contribute to the value. If a peer sends those padding bits as non-zero (malformed but not impossible), the stored `teid` is non-zero even though the actual 3 TEID bits are all zero. The downstream check `if (teid_octets && p.prefix.t2st_route.teid == 0)` then silently accepts a route that the draft requires to be treated as malformed. A mask of `(0xFFFFFFFFu << (32 - teid_bits)) & 0xFFFFFFFFu` applied after `ntohl` would isolate only the significant bits before the zero-check and before storage.
### Issue 2 of 2
bgpd/bgp_mup.c:167-185
**T2ST encoder: `teid_octets` is not bounded to 4 bytes**
`teid_octets = (teid_bits + 7) / 8` is computed directly from `endpoint_address_length` with no upper-bound guard. When `endpoint_address_length > addr_octets * 8 + 32` (e.g. `endpoint_address_length = 96` for IPv4), `teid_bits = 64` and `teid_octets = 8`, causing `stream_put(s, &teid_be, teid_octets)` to read 8 bytes from the 4-byte `uint32_t teid_be` — a stack buffer overread. The reflected-route path is safe today because the parser caps `ea_len` to 64/160, but origination code (planned for a follow-on PR) that directly populates `struct mup_t2st_route` would be exposed. A `teid_octets = MIN(teid_octets, 4u)` guard before the `stream_put` would close the gap.
Reviews (1): Last reviewed commit: "tests: add BGP-MUP topotest covering cap..." | Re-trigger Greptile |
| peer->bgp->vrf_id, peer->host, src_len, afi); | ||
| return -1; | ||
| } | ||
| ext->source_address_length = src_len; | ||
| if (src_len) { | ||
| src_octets = src_len / 8; | ||
| if (off + src_octets > psize) { | ||
| flog_err(EC_BGP_MUP_PACKET, "%u:%s - Rx BGP-MUP T1ST NLRI truncated source", | ||
| peer->bgp->vrf_id, peer->host); | ||
| return -1; | ||
| } | ||
| ext->source_address.ipa_type = (afi == AFI_IP) ? IPADDR_V4 : IPADDR_V6; | ||
| memcpy(&ext->source_address.ip.addr, pfx + off, src_octets); | ||
| } | ||
|
|
||
| if (attr) | ||
| bgp_update(peer, (struct prefix *)&p, addpath_id, attr, afi, safi, ZEBRA_ROUTE_BGP, | ||
| BGP_ROUTE_NORMAL, &prd, NULL, 0, 0, NULL); |
There was a problem hiding this comment.
T2ST partial-TEID padding bits not masked — TEID=0 check can be bypassed
When teid_bits % 8 != 0 (e.g., ea_len = 35 for IPv4 means 3 significant TEID bits and 5 padding bits in the last byte), teid_be is zero-initialised and then memcpy fills the low-address bytes on little-endian systems. After ntohl, the significant TEID bits land at the MSB, but the 5 padding bits also contribute to the value. If a peer sends those padding bits as non-zero (malformed but not impossible), the stored teid is non-zero even though the actual 3 TEID bits are all zero. The downstream check if (teid_octets && p.prefix.t2st_route.teid == 0) then silently accepts a route that the draft requires to be treated as malformed. A mask of (0xFFFFFFFFu << (32 - teid_bits)) & 0xFFFFFFFFu applied after ntohl would isolate only the significant bits before the zero-check and before storage.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_mup.c
Line: 393-410
Comment:
**T2ST partial-TEID padding bits not masked — TEID=0 check can be bypassed**
When `teid_bits % 8 != 0` (e.g., `ea_len = 35` for IPv4 means 3 significant TEID bits and 5 padding bits in the last byte), `teid_be` is zero-initialised and then `memcpy` fills the low-address bytes on little-endian systems. After `ntohl`, the significant TEID bits land at the MSB, but the 5 padding bits also contribute to the value. If a peer sends those padding bits as non-zero (malformed but not impossible), the stored `teid` is non-zero even though the actual 3 TEID bits are all zero. The downstream check `if (teid_octets && p.prefix.t2st_route.teid == 0)` then silently accepts a route that the draft requires to be treated as malformed. A mask of `(0xFFFFFFFFu << (32 - teid_bits)) & 0xFFFFFFFFu` applied after `ntohl` would isolate only the significant bits before the zero-check and before storage.
How can I resolve this? If you propose a fix, please make it concise.| */ | ||
| uint8_t teid_bits; | ||
| uint8_t teid_octets; | ||
| uint32_t teid_be; | ||
|
|
||
| addr_octets = (afi == AFI_IP) ? IPV4_MAX_BYTELEN : IPV6_MAX_BYTELEN; | ||
| teid_bits = (mp->t2st_route.endpoint_address_length > addr_octets * 8) | ||
| ? mp->t2st_route.endpoint_address_length - (addr_octets * 8) | ||
| : 0; | ||
| teid_octets = (teid_bits + 7) / 8; | ||
| teid_be = htonl(mp->t2st_route.teid); | ||
|
|
||
| stream_put(s, mp->rd, 8); | ||
| stream_putc(s, mp->t2st_route.endpoint_address_length); | ||
| stream_put(s, &mp->t2st_route.endpoint_address.ip.addr, addr_octets); | ||
| if (teid_octets) | ||
| stream_put(s, &teid_be, teid_octets); | ||
|
|
||
| total_len = 8 + 1 + addr_octets + teid_octets; |
There was a problem hiding this comment.
T2ST encoder:
teid_octets is not bounded to 4 bytes
teid_octets = (teid_bits + 7) / 8 is computed directly from endpoint_address_length with no upper-bound guard. When endpoint_address_length > addr_octets * 8 + 32 (e.g. endpoint_address_length = 96 for IPv4), teid_bits = 64 and teid_octets = 8, causing stream_put(s, &teid_be, teid_octets) to read 8 bytes from the 4-byte uint32_t teid_be — a stack buffer overread. The reflected-route path is safe today because the parser caps ea_len to 64/160, but origination code (planned for a follow-on PR) that directly populates struct mup_t2st_route would be exposed. A teid_octets = MIN(teid_octets, 4u) guard before the stream_put would close the gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_mup.c
Line: 167-185
Comment:
**T2ST encoder: `teid_octets` is not bounded to 4 bytes**
`teid_octets = (teid_bits + 7) / 8` is computed directly from `endpoint_address_length` with no upper-bound guard. When `endpoint_address_length > addr_octets * 8 + 32` (e.g. `endpoint_address_length = 96` for IPv4), `teid_bits = 64` and `teid_octets = 8`, causing `stream_put(s, &teid_be, teid_octets)` to read 8 bytes from the 4-byte `uint32_t teid_be` — a stack buffer overread. The reflected-route path is safe today because the parser caps `ea_len` to 64/160, but origination code (planned for a follow-on PR) that directly populates `struct mup_t2st_route` would be exposed. A `teid_octets = MIN(teid_octets, 4u)` guard before the `stream_put` would close the gap.
How can I resolve this? If you propose a fix, please make it concise.f083469 to
47e1179
Compare
47e1179 to
867b713
Compare
|
Dropped the "tests: topotests: support ExaBGP 5.x in TopoExaBGP.start" About the [CI] Basic Tests failure (the bgp_mup topotest on 4 |
| IANA_SAFI_ENCAP = 7, | ||
| IANA_SAFI_EVPN = 70, | ||
| IANA_SAFI_BGP_LS = 71, /* BGP-LS per RFC 9552 */ | ||
| IANA_SAFI_MUP = 85, /* BGP-MUP SAFI, IANA-assigned */ |
There was a problem hiding this comment.
aren't all of these IANA-assigned? This comment.... is just useless.
| #define AF_FLOWSPEC (AF_MAX + 2) | ||
| #endif | ||
|
|
||
| #if !defined(AF_MUP) |
There was a problem hiding this comment.
prefix code belongs in it's own commit.
| stream_putc(s, IPV4_MAX_BYTELEN); | ||
| stream_put_ipv4(s, attr->mp_nexthop_global_in.s_addr); | ||
| break; | ||
| case SAFI_MUP: |
There was a problem hiding this comment.
Adding the case here belongs here definately, but the stream_put code belongs in a later commit.
| const struct prefix_mup *mp = (const struct prefix_mup *)p; | ||
|
|
||
| /* Architecture Type(1) + Route Type(2) + Length(1) + Length octets. */ | ||
| return 4 + mp->prefix.length; |
There was a problem hiding this comment.
we do not use magic numbers
| if (src_octets) | ||
| stream_put(s, &e->source_address.ip.addr, src_octets); | ||
|
|
||
| total_len = 8 + 1 + prefix_octets + 4 + 1 + 1 + ep_octets + 1 + src_octets; |
There was a problem hiding this comment.
magic numbers magic numbers everywhere
| #define ECOMMUNITY_ENCODE_OPAQUE 0x03 | ||
| #define ECOMMUNITY_ENCODE_EVPN 0x06 | ||
| #define ECOMMUNITY_ENCODE_REDIRECT_IP_NH 0x08 /* Flow Spec */ | ||
| #define ECOMMUNITY_ENCODE_MUP 0x0c |
There was a problem hiding this comment.
please follow formatting of the rest of the file
| extern int bgp_nlri_parse_mup(struct peer *peer, struct attr *attr, struct bgp_nlri *packet, | ||
| bool withdraw); | ||
|
|
||
| /* Emit the decomposed MUP NLRI fields (routeType / archType / rd / ip / |
There was a problem hiding this comment.
looks like ai verbiage is way too much. Rework as if a human would want to read it and it follows the rest of the style of bgpd. This is a generic comment for all of the code
| json_object_object_add(json, "nlri", json_nlri); | ||
| } | ||
| } else if (p->family == AF_MUP) { | ||
| /* inet_ntop() has no AF_MUP renderer and returns NULL, which |
There was a problem hiding this comment.
neither does AF_FLOWSPEC or AF_ETHERNET. Why do we need this comment?
|
more than a bit of rework is needed here. Please also follow the frrbot suggestions |
Add the AF_MUP address family and the MUP route-type prefix structures (struct mup_prefix and the per-route-type bodies) used to carry a BGP-MUP route key, and define SAFI_MUP=9 with wire codepoint 85. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Add SAFI_MUP arms to the existing safi_t switches to keep -Wswitch-enum clean. Protocol-specific logic is marked with TODO placeholders and implemented in subsequent commits. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Introduce bgp_mup.{c,h} with the encode/decode for the four
draft-ietf-bess-mup-safi route types (3gpp-5g architecture).
A malformed NLRI of a known route type is treated as withdraw per
RFC 7606 and skipped; unknown architecture or route types are
ignored. Only a corrupt outer header resets the session.
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Cover the BGP-MUP NLRI codec: parse of all four route types for both AFIs, the malformed-NLRI treat-as-withdraw and skip paths, and an encode/parse round-trip byte-compared against the same vectors. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
draft-ietf-bess-mup-safi 3.2 defines a transitive extended community carrying a Direct-Type Segment Identifier for Type 2 ST routes. The type codepoint 0x0c is the IANA-assigned "SRv6 MUP Extended Community"; the sub-type 0x00 follows existing implementations. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Register BGP_MUPV4_NODE / BGP_MUPV6_NODE entered through "address-family ipv4|ipv6 mup"; draft-ietf-bess-mup-safi uses AFI=1 and AFI=2 so each AFI gets its own VTY node. The nodes accept "neighbor activate" and the per-neighbor policy commands so peering can be enabled and filtered per-AFI. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
route_vty_out_route() renders prefixes via inet_ntop(), which has no AF_MUP formatter; use the MUP %pFX printer instead, and decompose the NLRI into structured fields via bgp_mup_route2json() for JSON output. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Document the BGP-MUP SAFI (draft-ietf-bess-mup-safi): the four MUP route types, how "address-family ipv4|ipv6 mup" activates the sub-AFIs on a peering, what the speaker does with received routes, and the show commands for inspecting them. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Verify the BGP-MUP control plane over a live topology: capability negotiation for both AFIs, parse of all four route types announced by an ExaBGP peer, show output, and re-advertisement and withdraw to a second FRR speaker that exercise the encode and withdraw paths on the wire. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
867b713 to
c03010f
Compare
Summary
Add control-plane support for the BGP Mobile User Plane (BGP-MUP) SAFI
defined in
draft-ietf-bess-mup-safi,
used to distribute session state for the SRv6 Mobile User Plane
architecture (RFC 9433). It includes:
address-family ipv4|ipv6 mupshow bgp <ipv4|ipv6> mup [all] [json]with structured JSON fieldspeers with ExaBGP (an independent implementation of the draft)
This makes FRR usable as a route reflector / route server for BGP-MUP.
Route origination and FIB installation are intentionally not included:
origination follows in a separate PR, and FIB installation depends on
Linux seg6 Mobile User Plane UAPI that is not yet upstream.
Related Issue
None.
Components
bgpd, vtysh, lib, doc, tests