[PATCH net] openvswitch: advance by the real header length in get_ipv6_ext_hdrs()

[PATCH net] openvswitch: advance by the real header length in get_ipv6_ext_hdrs()

[Joas Antonio dos Santos]

get_ipv6_ext_hdrs() walks the IPv6 extension header chain to fill
key->ipv6.exthdrs, but advances to the next header with ipv6_optlen()
whatever the current header is.  That is only right for Hop-by-Hop,
Destination Options and Routing headers:

 - an Authentication Header is ((hdrlen + 2) << 2) bytes long
   (ipv6_authlen()), so with hdrlen = 4 the walk skips 40 bytes instead
   of 24 and reads the next "header" from the ICV and payload;

 - a Fragment header is always 8 bytes, and the byte read as hdrlen is
   its Reserved field, which the sender controls and receivers ignore,
   so a non-zero value makes the walk skip up to 2048 bytes and miss
   the headers that follow.

In both cases the OFPIEH12_* bits stored in key->ipv6.exthdrs depend on
bytes chosen by the sender rather than on the headers that are actually
present: payload bytes that look like another AH or a Hop-by-Hop header
add OFPIEH12_UNREP/OFPIEH12_UNSEQ, and real headers after a Fragment
header can go unreported.  Flows that match on OVS_KEY_ATTR_IPV6_EXTHDRS
then classify such packets wrongly.  There is no memory safety issue,
skb_header_pointer() bounds every read.

Advance by sizeof(struct frag_hdr) for Fragment headers and by
ipv6_authlen() for AH, as ipv6_find_hdr() does.

Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
Link: https://lore.kernel.org/all/178905555085.219967.6617611783437228045@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
Found while going through the pre-existing issues the Sashiko review of
"openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()" pointed out
(Link above); Eelco mentioned putting them on his todo list, and I did
not find a fix for this part in net, net-next or on the list.  Happy to
drop this if a fix is already in flight.

Tested by building get_ipv6_ext_hdrs() before and after this patch in a
userspace harness (skb_header_pointer() replaced by a bounds-checked
memcpy from a buffer) and feeding it crafted packets:

  AH(hdrlen=4) + DSTOPTS + TCP, payload mimicking AH at old offset
    before: AUTH DEST UNREP UNSEQ      after: AUTH DEST UNSEQ
  FRAG(Reserved=0xff) + DSTOPTS + ROUTING + TCP
    before: DEST FRAG UNSEQ            after: DEST FRAG ROUTER UNSEQ
  FRAG(Reserved=0) + ROUTING + TCP, HOP + DST + ROUTING + TCP
    unchanged

net/openvswitch/ builds cleanly with W=1 (arm64, net/main 94cea342e891).
I did not test it on a live datapath.

 net/openvswitch/flow.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 1c4f3a079..a1c60d2f5 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -372,8 +372,13 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
 		hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
 		if (!hp)
 			break;
+		if (next_type == IPPROTO_FRAGMENT)
+			start += sizeof(struct frag_hdr);
+		else if (next_type == IPPROTO_AH)
+			start += ipv6_authlen(hp);
+		else
+			start += ipv6_optlen(hp);
 		next_type = hp->nexthdr;
-		start += ipv6_optlen(hp);
 	}
 }
 
-- 
2.39.5

Re: [PATCH net] openvswitch: advance by the real header length in get_ipv6_ext_hdrs()

[Ilya Maximets]

On 10/29/26 4:53 PM, Joas Antonio dos Santos wrote:
> get_ipv6_ext_hdrs() walks the IPv6 extension header chain to fill
> key->ipv6.exthdrs, but advances to the next header with ipv6_optlen()
> whatever the current header is.  That is only right for Hop-by-Hop,
> Destination Options and Routing headers:
> 
>  - an Authentication Header is ((hdrlen + 2) << 2) bytes long
>    (ipv6_authlen()), so with hdrlen = 4 the walk skips 40 bytes instead
>    of 24 and reads the next "header" from the ICV and payload;
> 
>  - a Fragment header is always 8 bytes, and the byte read as hdrlen is
>    its Reserved field, which the sender controls and receivers ignore,
>    so a non-zero value makes the walk skip up to 2048 bytes and miss
>    the headers that follow.
> 
> In both cases the OFPIEH12_* bits stored in key->ipv6.exthdrs depend on
> bytes chosen by the sender rather than on the headers that are actually
> present: payload bytes that look like another AH or a Hop-by-Hop header
> add OFPIEH12_UNREP/OFPIEH12_UNSEQ, and real headers after a Fragment
> header can go unreported.  Flows that match on OVS_KEY_ATTR_IPV6_EXTHDRS
> then classify such packets wrongly.  There is no memory safety issue,
> skb_header_pointer() bounds every read.
> 
> Advance by sizeof(struct frag_hdr) for Fragment headers and by
> ipv6_authlen() for AH, as ipv6_find_hdr() does.
> 
> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
> Link: https://lore.kernel.org/all/178905555085.219967.6617611783437228045@kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
> Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5

The tag is no longer needed.  And it has a wrong format anyway.

> ---
> Found while going through the pre-existing issues the Sashiko review of
> "openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()" pointed out
> (Link above); Eelco mentioned putting them on his todo list, and I did
> not find a fix for this part in net, net-next or on the list.  Happy to
> drop this if a fix is already in flight.
> 
> Tested by building get_ipv6_ext_hdrs() before and after this patch in a
> userspace harness (skb_header_pointer() replaced by a bounds-checked
> memcpy from a buffer) and feeding it crafted packets:
> 
>   AH(hdrlen=4) + DSTOPTS + TCP, payload mimicking AH at old offset
>     before: AUTH DEST UNREP UNSEQ      after: AUTH DEST UNSEQ
>   FRAG(Reserved=0xff) + DSTOPTS + ROUTING + TCP
>     before: DEST FRAG UNSEQ            after: DEST FRAG ROUTER UNSEQ
>   FRAG(Reserved=0) + ROUTING + TCP, HOP + DST + ROUTING + TCP
>     unchanged
> 
> net/openvswitch/ builds cleanly with W=1 (arm64, net/main 94cea342e891).
> I did not test it on a live datapath.
> 
>  net/openvswitch/flow.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 1c4f3a079..a1c60d2f5 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -372,8 +372,13 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
>  		hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
>  		if (!hp)
>  			break;
> +		if (next_type == IPPROTO_FRAGMENT)
> +			start += sizeof(struct frag_hdr);

Should we also stop it's not a first fragment?  There is no actual
next header after the fragment one if it's not the first.  Otherwise,
the patch seems incomplete.

Best regards, Ilya Maximets.

Re: [PATCH net] openvswitch: advance by the real header length in get_ipv6_ext_hdrs()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0 (2)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete for non-first fragments.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but OFPIEH12_ESP
  is never set.
- [Low] This problem wasn't introduced by this patch, but a single
  Destination Options header in the final-destination position is…
- [Low] This problem wasn't introduced by this patch, but repeated Hop-
  by-Hop headers are not reported as OFPIEH12_UNREP when the first one…

--

openvswitch: advance by the real header length in get_ipv6_ext_hdrs()

get_ipv6_ext_hdrs() used to move past every extension header with
ipv6_optlen(). That length is wrong for AH and Fragment headers. This
patch advances by sizeof(struct frag_hdr) for Fragment headers and by
ipv6_authlen() for AH, so the OFPIEH12_* bits come from the headers that
are actually present.

Link: https://lore.kernel.org/all/178905555085.219967.6617611783437228045@kernel.org/

> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 1c4f3a0790443..a1c60d2f552cf 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -372,8 +372,13 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
>  		hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
>  		if (!hp)
>  			break;
> +		if (next_type == IPPROTO_FRAGMENT)
> +			start += sizeof(struct frag_hdr);
> +		else if (next_type == IPPROTO_AH)
> +			start += ipv6_authlen(hp);
> +		else
> +			start += ipv6_optlen(hp);
> +		next_type = hp->nexthdr;

[Severity: High]
Can payload bytes still set the exthdrs bits when the packet is a
non-first fragment?

The IPPROTO_FRAGMENT case in get_ipv6_ext_hdrs() sets OFPIEH12_FRAG and
never reads frag_off. This branch then always advances 8 bytes and
follows hp->nexthdr.

RFC 8200 says every fragment carries the Next Header value of the first
header in the Fragmentable Part. Take IPsec AH transport mode with
fragmentation as an example. A later fragment shows up as
FRAG(offset != 0, nexthdr = AH).

The bytes after that Fragment header are payload from the middle of the
packet. The next loop iteration still parses them as an ipv6_opt_hdr:
payload[0] is taken as the next type and payload[1] as the length.
Payload bytes such as 0, 43, 44, 51 or 59 would then set HOP/UNSEQ,
ROUTER, FRAG/UNREP, AUTH/UNREP or NONEXT for headers that are not there.

The commit message says the walk now advances "as ipv6_find_hdr() does".
However, ipv6_find_hdr() stops at non-first fragments:

net/ipv6/exthdrs_core.c:ipv6_find_hdr() {
    ...
			_frag_off = ntohs(*fp) & ~0x7;
			if (_frag_off) {
				if (target < 0 &&
				    ((!ipv6_ext_hdr(hp->nexthdr)) ||
				     hp->nexthdr == NEXTHDR_NONE)) {
					if (fragoff)
						*fragoff = _frag_off;
					return hp->nexthdr;
				}
				if (!found)
					return -ENOENT;
    ...
}

The wrong bits also seem to reach the flow key. parse_ipv6hdr() calls
get_ipv6_ext_hdrs() before ipv6_find_hdr(). In the
FRAG(offset != 0, nexthdr = extension header) case, ipv6_find_hdr()
returns -ENOENT and leaves frag_off at 0. parse_ipv6hdr() therefore
sets OVS_FRAG_TYPE_FIRST and returns -EPROTO. key_extract_l3l4() then
treats that as success:

net/openvswitch/flow.c:key_extract_l3l4() {
    ...
			case -EPROTO:
				skb->transport_header = skb->network_header;
				error = 0;
				break;
    ...
}

key->ipv6.exthdrs is never cleared on this path. Flows that match on
OVS_KEY_ATTR_IPV6_EXTHDRS would still misclassify these packets. The
tests in the changelog only cover Fragment headers with offset 0.

Would it make sense for the IPPROTO_FRAGMENT case to read frag_off with
skb_header_pointer() and stop the walk when ntohs(frag_off) & ~0x7 is
non-zero?

>  	}
>  }
>

[Severity: Medium]
This is a pre-existing issue and the patch doesn't introduce it, but is
OFPIEH12_ESP ever set? The loop in get_ipv6_ext_hdrs() is guarded by:

	while (ipv6_ext_hdr(next_type)) {

ipv6_ext_hdr() returns true only for HOP, ROUTING, FRAGMENT, AUTH, NONE
and DEST:

net/ipv6/exthdrs_core.c:ipv6_ext_hdr() {
	return   (nexthdr == NEXTHDR_HOP)	||
		 (nexthdr == NEXTHDR_ROUTING)	||
		 (nexthdr == NEXTHDR_FRAGMENT)	||
		 (nexthdr == NEXTHDR_AUTH)	||
		 (nexthdr == NEXTHDR_NONE)	||
		 (nexthdr == NEXTHDR_DEST);
}

NEXTHDR_ESP is not in that list, so the loop exits before case
IPPROTO_ESP can run. flow.h documents OFPIEH12_ESP as "Encrypted Sec
Payload header present", and the bit is exported through
OVS_KEY_ATTR_IPV6_EXTHDRS. A flow matching ESP=1 can never match, and a
flow matching ESP=0 matches every ESP packet. The OFPIEH12_ESP check in
the second DSTOPTS mask is dead code for the same reason.

If the loop guard is ever changed to include ESP, the new length
dispatch in this patch would send ESP to ipv6_optlen(hp). That reads SPI
bytes as a length, so ESP would need an explicit stop there.

This patch does not change ESP behaviour. Before the patch, an AH
followed by ESP also reached next_type == ESP, because next_type comes
from the AH header's own nexthdr field.

[Severity: Low]
This isn't a bug introduced by this patch, but should a single
Destination Options header in the final-destination position really get
OFPIEH12_UNSEQ? The first DSTOPTS seen in get_ipv6_ext_hdrs() is only
accepted if it comes after HOP:

		case IPPROTO_DSTOPTS:
			if (dest_options_header_count == 0) {
				if (*ext_hdrs &
				    ~(OFPIEH12_HOP | OFPIEH12_UNREP))
					*ext_hdrs |= OFPIEH12_UNSEQ;

Take IPv6 -> Routing -> DSTOPTS -> TCP, or AH -> DSTOPTS. ROUTER or AUTH
is already set by then, so UNSEQ is set. The changelog's own test result
shows this: "AUTH DEST UNSEQ".

The comment above the function lists the preferred order as Hop, Dest,
Routing, Fragment, AH, ESP, Dest, upper-layer. Routing -> Dest and
AH -> Dest both follow that order. The count of DSTOPTS headers seen so
far is being used as if it were the header's position.

[Severity: Low]
This is a pre-existing issue, but are repeated Hop-by-Hop headers missed
when the first one is out of sequence? In get_ipv6_ext_hdrs():

		case IPPROTO_HOPOPTS:
			if (*ext_hdrs & OFPIEH12_HOP)
				*ext_hdrs |= OFPIEH12_UNREP;
			...
			if (*ext_hdrs == 0)
				*ext_hdrs |= OFPIEH12_HOP;
			else
				*ext_hdrs |= OFPIEH12_UNSEQ;

OFPIEH12_HOP is only set when HOP is the first header. Take
IPv6 -> Routing -> HOP -> HOP -> TCP. The first HOP sees ROUTER already
set, so it sets UNSEQ but not HOP. The second HOP finds HOP clear and
also sets only UNSEQ.

UNREP stays clear even though two HOP headers are present.
ipv6_find_hdr() checks neither order nor repetition, so a short chain
like this passes and the missing bit stays in the flow key.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179328922134.76862.10417963652853801970%40gmail.com