Re: [PATCH net v4] ipv6: Implement limits on extension header parsing

[Justin Iurman <justin.iurman@gmail.com>]

On 4/28/26 20:16, Daniel Borkmann wrote:
> On 4/28/26 7:58 PM, Justin Iurman wrote:
>> On 4/28/26 17:37, Daniel Borkmann wrote:
>>> ipv6_{skip_exthdr,find_hdr}() and ip6_{tnl_parse_tlv_enc_lim,
>>> protocol_deliver_rcu}() iterate over IPv6 extension headers until they
>>> find a non-extension-header protocol or run out of packet data. The
>>> loops have no iteration counter, relying solely on the packet length
>>> to bound them. For a crafted packet with 8-byte extension headers
>>> filling a 64KB jumbogram, this means a worst case of up to ~8k
>>> iterations with a skb_header_pointer call each. ipv6_skip_exthdr(),
>>> for example, is used where it parses the inner quoted packet inside
>>> an incoming ICMPv6 error:
>>>
>>>    - icmpv6_rcv
>>>      - checksum validation
>>>      - case ICMPV6_DEST_UNREACH
>>>        - icmpv6_notify
>>>          - pskb_may_pull()       <- pull inner IPv6 header
>>>          - ipv6_skip_exthdr()    <- iterates here
>>>          - pskb_may_pull()
>>>          - ipprot->err_handler() <- sk lookup
>>>
>>> The per-iteration cost of ipv6_skip_exthdr itself is generally
>>> light, but skb_header_pointer becomes more costly on reassembled
>>> packets: the first ~1232 bytes of the inner packet are in the skb's
>>> linear area, but the remaining ~63KB are in the frag_list where
>>> skb_copy_bits is needed to read data.
>>>
>>> Initially, the idea was to add a configurable limit via a new
>>> sysctl knob with default 8, in line with knobs from commit
>>> 47d3d7ac656a ("ipv6: Implement limits on Hop-by-Hop and Destination
>>> options"), but two reasons eventually argued against it:
>>>
>>> - It adds to UAPI that needs to be maintained forever, and
>>>    upcoming work is restricting extension header ordering anyway,
>>>    leaving little reason for another sysctl knob
>>> - exthdrs_core.c is always built-in even when CONFIG_IPV6=n,
>>>    where struct net has no .ipv6 member, so the read site would
>>>    need an ifdef'd fallback to a constant anyway
>>>
>>> Therefore, just use a constant (IP6_MAX_EXT_HDRS_CNT). All four
>>> extension header walking functions are now bound by this limit.
>>>
>>> Note that the check in ip6_protocol_deliver_rcu() happens right
>>> before the goto resubmit, such that we don't have to have a test
>>> for ipv6_ext_hdr() in the fast-path.
>>>
>>> There's an ongoing IETF draft-iurman-6man-eh-occurrences to enforce
>>> IPv6 extension headers ordering and occurrence. The latter also
>>> discusses security implications. As per RFC8200 section 4.1, the
>>> occurrence rules for extension headers provide a practical upper
>>> bound which is 8. In order to be conservative, let's define
>>> IP6_MAX_EXT_HDRS_CNT as 4x that to leave enough room for quirky
>>> setups. In the unlikely event that this is still not enough, then
>>> we might need to reconsider a sysctl.
>>>
>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>
>> Reviewed-by: Justin Iurman <justin.iurman@gmail.com>
>>
>> FWIW, I prefer v4 over v3. I didn't like the idea of a new sysctl for 
>> several reasons I already mentioned. So, thanks for this new version, 
>> Daniel.
>>
>> However, I can't help but think 8 would pose absolutely no problem. I 
>> would be very surprised to see someone complain. We're choosing 32 
>> over 8 to be extra safe, at the price of security. RFC8200 provides 
>> the following ordered list which is recommended for senders (without 
>> normative language, though, which was a mistake):
>>
>>        Hop-by-Hop Options header
>>        Destination Options header
>>        Routing header
>>        Fragment header
>>        Authentication header
>>        Encapsulating Security Payload header
>>        Destination Options header
>>
>> So this is the maximum you can have theoretically***, although you 
>> wouldn't for instance use the Authentication header with ESP. I'm not 
>> even talking about ordering or specific number of occurrences here, 
>> just the total number of Extension Headers in a packet (as your patch 
>> does). It's also worth mentioning that it's highly unlikely to see 
>> someone use them all at the same time (in production, of course). This 
>> is why I still think that 8 is safe too, and would provide security as 
>> expected.
> 
> What do you think if we fix this at 12 then to also have the exotic cases
> below covered just in case to be conservative? Would still be an 
> improvement
> over 32 fwiw. I'm also fine if you think straight to 8 is the better 
> choice.
> Either option I can spin a v5, np.

I'm fine either way. IMO, 12 seems like a good compromise to keep 
everyone happy.