Page MenuHomeFreeBSD

ipfw: prefixlen segfault bugfix in nptv6
AcceptedPublic

Authored by p.mousavizadeh_protonmail.com on May 29 2025, 2:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 8, 3:51 PM
Unknown Object (File)
Mon, Jul 7, 5:13 PM
Unknown Object (File)
Mon, Jul 7, 12:06 PM
Unknown Object (File)
Tue, Jul 1, 6:43 PM
Unknown Object (File)
Tue, Jul 1, 6:43 AM
Unknown Object (File)
Mon, Jun 30, 12:08 PM
Unknown Object (File)
Sun, Jun 29, 8:47 PM
Unknown Object (File)
Sun, Jun 29, 8:39 AM

Details

Reviewers
glebius
ivy
olivier
ae
adrian
des
kevans
Group Reviewers
network
Summary

I found a bug in the ipfw implementation of NPTv6, and it seems that it originated from cleaning up compiler warnings in 2020 (D25456). The bug occurs when we set a prefix with a prefix length inside the ipfw nptv6 create, which leads to a segmentation fault. The segmentation fault is caused by a goto statement that dereferences a null pointer.

I wanted to fix that, but I have to break backward compatibility due to poor design. It gives the user the option to set the prefix length both inside the prefix and via the prefix length option, which is confusing (D6420#142790). Additionally, using them in the incorrect order would result in undefined behavior.

So, should I remove the prefix length options or replace the internal and external prefix length addresses with their network identifier address, which is the first address of the resulting prefix?
This implementation and my preference is for the second approach because, in NPTv6, you have to set the same prefix length for both internal and external IPv6 prefixes due to its stateless nature (RFC 6296 Sec. 3.1).
Also, since we will broke the compatibility in anyway to fix this, should I change the int_prefix and ext_prefix to int_netaddr and ext_netaddr to be less confusing?

Test Plan

The bug without this fix:

# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_prefix 2a01:e140::/64
Segmentation fault (core dumped)
# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_if vmx0
Segmentation fault (core dumped)
# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_if vmx0 prefixlen 64
Segmentation fault (core dumped)

Here is how I tested the new code:

# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140:: prefixlen 64
# ipfw nptv6 test show                                                                  
nptv6 test int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140:: prefixlen 64
# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::/64 prefixlen 64
ipfw: Bad IPv6 network address: 2a01:e140::/64
Use prefixlen option instead
# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_prefix 2a01:e140:: prefixlen 64 
ipfw: Bad IPv6 network address: 2a01:e140:cafe::/64
Use prefixlen option instead
# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::                
ipfw: prefixlen required
# ipfw nptv6 test create prefixlen 64 int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::
# ipfw nptv6 test show                                                                     
nptv6 test int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140:: prefixlen 64

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 64529
Build 61413: arc lint + arc unit

Event Timeline

I think this looks fine. I'm personally not too worried about breaking things in ipfw to make them much cleaner/clearer/less error prone, as long as we document it in UPDATING.

This revision is now accepted and ready to land.Jun 1 2025, 3:34 AM

usually i prefer CIDR prefixes over prefixlen, but since the same mask applies to both networks, i think prefixlen is better here.

the fact that no one noticed this for 5 years suggests the other syntax isn't commonly used, but it probably needs a Relnotes entry since we're breaking a (theoretically) previously supported syntax.

Thank you all for the reviews.
At this point, I think this revision is ready to land. @ivy, Could you please commit it? is there anything I can help with or do to make it happen?