Skip to content

Fix handling of RFC6603 Prefix Exclude Option#151

Open
mmmspatz wants to merge 1 commit intoopenwrt:masterfrom
mmmspatz:mspatz/fix_exclude
Open

Fix handling of RFC6603 Prefix Exclude Option#151
mmmspatz wants to merge 1 commit intoopenwrt:masterfrom
mmmspatz:mspatz/fix_exclude

Conversation

@mmmspatz
Copy link

@mmmspatz mmmspatz commented Jan 18, 2026

The encoding and decoding of option DHCPV6_OPT_PD_EXCLUDE both had several critical bugs.

The encoding bugs are less important, because while the RFC specifies that requesting routers echo back excluded prefixes in RELEASE messages, it seems unlikely that a server would have any use for this information. But the decoding bugs render the feature completely unusable.

I have spent frankly an embarrassing number of hours pondering the correct behavior as specified by the RFC, and I'm pretty sure I've got it.
I tried to be more explicit with variable names and comments for clarity.

I discovered this because I am working on adding support for DHCPV6_OPT_PD_EXCLUDE to odhcpd, I will make a PR there soon.

In the encoding step in dhcpv6_send():

  1. The for loop on line 844 had an off by one error: bytes were copied into ia_pd[] one too far to the right ( k went from ex_len - 5 downto 1.)
  2. The left shift on line 842 incorrectly shifts by one byte if pd_entries[j].exclusion_length - pd_entries[j].length) % 8 == 0

In the decoding step in dhcpv6_parse_ia():

  1. Options with slen == 2 (valid, indicates delegated length - exclusion_length <= 8) are incorrectly ignored.
  2. The check for if (slen <= bytes_needed) on line 1828 should be if (slen - 1 <= bytes_needed) (slen includes one byte for the prefix-len field)
  3. The do-while loop on 1836 copies the subnet-id field from sdata[] in the wrong byte order (it's network order, MSB first) looking again, the order is correct. Looking for a third time, the order is indeed wrong.
  4. Again, the right shift on line 1840 incorrectly shifts by one byte if (exclude_length - entry.length) % 8 == 0

@systemcrash
Copy link
Contributor

You took a similar approach to how I did; when I started looking at this same problem, I began wondering whether we should just do 128 bit math, and skip the prefix/subnet handling altogether (especially when bit lengths approach those boundaries), since that means 4 blocks of 32 bits or 2 blocks of 64 bits. The restriction we hit for a single 128 bit uint is that the older 32 bit platform CPUs openwrt supports might not readily support a whole 128 bit uint. So .. two 64s it is, as you have here.

The math in the original RFC goes with the whole 128. Because that's how this option is supposed to work - independent of mandates today ("a prefix is max 64 blah blah"), and just deal with the whole address length. It looks easy, and should be, but...

I'll take another look with fresh eyes soon - I'm glad to see this is addressed by others in the community.

@mmmspatz
Copy link
Author

Honestly I didn't even try to stretch my brain past the "32 bit hostid" limitation.
It seems fine in the sense that

  1. no user of openwrt needs to delegate a /31 with dhcpv6, and
  2. excluding a whole /64 when you were asked to exclude a /128 is acceptable (which I believe this does), though I the exclusion we echo back in the RELEASE would technically be wrong.

I did some testing of this against the RFC's pseudocode. It's messy, but if you're interested: https://github.com/mmmspatz/dhcp6_opt_pd_exlude_test

@mmmspatz
Copy link
Author

mmmspatz commented Jan 20, 2026

Mostly for fun, here is an example of how we could support exclusions up to /96 by using uint64_t:
mmmspatz/dhcp6_opt_pd_exlude_test@main...exlude_96

Several bugs in the encoding and (more importantly) decoding of
DHCPV6_OPT_PD_EXCLUDE lead to the option being ignored in some received
messages, or the excluded subnet id being mangled to an incorrect value.

Fix both encoding and decoding, being more explicit and slightly more
verbose for clarity.

Signed-off-by: Mark H. Spatz <mark.h.spatz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants