MPLS label endianness is inconsistent in bgpd

FRRouting / frr

The FRRouting Protocol Suite

https://frrouting.org/

Other

3.21k stars 1.24k forks source link

MPLS label endianness is inconsistent in bgpd #6129

Open eqvinox opened 4 years ago

eqvinox commented 4 years ago

host byte order: https://github.com/FRRouting/frr/blob/master/lib/mpls.h#L121 network byte order: https://github.com/FRRouting/frr/blob/master/bgpd/bgp_label.h#L55-L98

This breaks quite badly on big-endian systems, resulting in bugged routes like this:

B   0.0.0.0/0 [200/0] via 10.132.255.1 (recursive), label 1048543, 00:00:53
                        via 10.132.253.74, eth2, label 1048543, 00:00:53

Note 1048543 = 0xfffdf.

I'm not sure what exactly was intended here and/or which part of the code collided with that.

eqvinox commented 4 years ago

mpls_label_t is defined in lib/mpls.h and the header file consistently presumes labels are in host byte order. That lends the conclusion bgpd is misusing the API and labels should always be in host byte order.

eqvinox commented 4 years ago

so.

label_ntop / label_pton need to be removed
locations that need/want the label in network byte order need to use mpls_lse_t instead of mpls_label_t
all call locations of label_ntop / label_pton need to be audited and replaced with mpls_lse_encode / mpls_lse_decode where appropriate. Something is definitely wrong.

Note:

mpls_label_t is host byte order and does not contain the BOS bit
mpls_lse_t is network byte order and does contain the BOS bit

bgpd seems to be pushing values around including the BOS bit so presumably the most consistent would be to use mpls_lse_t where that is needed.

eqvinox commented 4 years ago

Any use of MPLS_INVALID_LABEL in bgpd is probably suspect since that constant is in host byte order, so assigning it to bgpd's network byte order fields would be wrong.