Re: [PATCH net v2] ipv6: Apply max_dst_opts_cnt to ip6_tnl_parse_tlv_enc_lim

[Daniel Borkmann <daniel@iogearbox.net>]

On 4/18/26 2:40 PM, Justin Iurman wrote:
> On 4/18/26 14:15, Daniel Borkmann wrote:
>> Commit 47d3d7ac656a ("ipv6: Implement limits on Hop-by-Hop and
>> Destination options") added net.ipv6.max_{hbh,dst}_opts_{cnt,len}
>> and applied them in ip6_parse_tlv(), the generic TLV walker
>> invoked from ipv6_destopt_rcv() and ipv6_parse_hopopts().
>>
>> ip6_tnl_parse_tlv_enc_lim() does not go through ip6_parse_tlv();
>> it has its own hand-rolled TLV scanner inside its NEXTHDR_DEST
>> branch which looks for IPV6_TLV_TNL_ENCAP_LIMIT. That inner
>> loop is bounded only by optlen, which can be up to 2048 bytes.
>> Stuffing the Destination Options header with 2046 Pad1 (type=0)
>> entries advances the scanner a single byte at a time, yielding
>> ~2000 TLV iterations per extension header.
>>
>> Reuse max_dst_opts_cnt to bound the TLV iterations, matching
>> the semantics from 47d3d7ac656a.
>>
>> Fixes: 47d3d7ac656a ("ipv6: Implement limits on Hop-by-Hop and Destination options")
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>>   v1->v2:
>>     - Remove unlikely (Justin)
>>     - Use abs() given max_dst_opts_cnt's negative meaning (Justin)
>>
>>   net/ipv6/ip6_tunnel.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
>> index 907c6a2af331..0f50b7fcb24e 100644
>> --- a/net/ipv6/ip6_tunnel.c
>> +++ b/net/ipv6/ip6_tunnel.c
>> @@ -430,11 +430,16 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
>>                   break;
>>           }
>>           if (nexthdr == NEXTHDR_DEST) {
>> +            int tlv_max = abs(READ_ONCE(init_net.ipv6.sysctl.max_dst_opts_cnt));
>> +            int tlv_cnt = 0;
>>               u16 i = 2;
>>               while (1) {
>>                   struct ipv6_tlv_tnl_enc_lim *tel;
>> +                if (tlv_cnt++ >= tlv_max)
>> +                    break;
>> +
>>                   /* No more room for encapsulation limit */
>>                   if (i + sizeof(*tel) > optlen)
>>                       break;
> 
> Thanks for v2, Daniel.
> 
> I'm still wondering: should we align the above parsing behavior with the one in ip6_parse_tlv() to keep things consistent? That is: don't increment tlv_cnt for Pad1/PadN, make sure we don't exceed 8 bytes per padding (consecutive Pad1's, or a PadN), and we could also check that a PadN payload is only made of zeroes. Open question...

Hm, so it would be sth like the below on top of this one, I'd wish we wouldn't
have to open code another ip6_parse_tlv().

Have you seen such cases with the encap limit in the wild (vs encap limit as
first tlv) where a stack places leading Pad1/PadN before encap limit which the
v2 patch wouldn't catch?

  net/ipv6/ip6_tunnel.c | 42 +++++++++++++++++++++++++++++++-----------
  1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 0f50b7fcb24e..1fa42a1cd97c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -432,28 +432,48 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
  		if (nexthdr == NEXTHDR_DEST) {
  			int tlv_max = abs(READ_ONCE(init_net.ipv6.sysctl.max_dst_opts_cnt));
  			int tlv_cnt = 0;
+			int padlen = 0;
  			u16 i = 2;
  
-			while (1) {
-				struct ipv6_tlv_tnl_enc_lim *tel;
+			while (i < optlen) {
+				struct ipv6_tlv_tnl_enc_lim *tel =
+					(struct ipv6_tlv_tnl_enc_lim *)(skb->data + off + i);
+				int tel_len;
  
-				if (tlv_cnt++ >= tlv_max)
+				if (tel->type == IPV6_TLV_PAD1) {
+					if (++padlen > 7)
+						break;
+					i++;
+					continue;
+				}
+
+				if (i + 2 > optlen)
+					break;
+				tel_len = tel->length + 2;
+				if (i + tel_len > optlen)
  					break;
  
-				/* No more room for encapsulation limit */
-				if (i + sizeof(*tel) > optlen)
+				if (tel->type == IPV6_TLV_PADN) {
+					padlen += tel_len;
+					if (padlen > 7)
+						break;
+					if (memchr_inv((u8 *)tel + 2, 0,
+						       tel->length))
+						break;
+					i += tel_len;
+					continue;
+				}
+				padlen = 0;
+
+				if (tlv_cnt++ >= tlv_max)
  					break;
  
-				tel = (struct ipv6_tlv_tnl_enc_lim *)(skb->data + off + i);
  				/* return index of option if found and valid */
  				if (tel->type == IPV6_TLV_TNL_ENCAP_LIMIT &&
  				    tel->length == 1)
  					return i + off - nhoff;
-				/* else jump to next option */
-				if (tel->type)
-					i += tel->length + 2;
-				else
-					i++;
+
+				i += tel_len;
  			}
  		}
  		nexthdr = hdr->nexthdr;
-- 
2.43.0

> Otherwise, LGTM:
> Reviewed-by: Justin Iurman <justin.iurman@gmail.com>

Thanks,
Daniel