[PATCH net v2] openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()

[PATCH net v2] openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()

[Eelco Chaudron]

The ESP and AH cases in get_ipv6_ext_hdrs() used IPPROTO_FRAGMENT instead
of OFPIEH12_FRAG when checking for out-of-order extension headers, causing
the fragment header to not be recognised as a valid predecessor.

The original code used IPPROTO_FRAGMENT (44) as a bitmask constant where
OFPIEH12_FRAG (1 << 4 = 16) was intended.  IPPROTO_FRAGMENT encodes bits
2, 3 and 5 (OFPIEH12_AUTH | OFPIEH12_DEST | OFPIEH12_ROUTER), but not
bit 4 (OFPIEH12_FRAG).  This caused incorrect OFPIEH12_UNSEQ verdicts in
both the ESP and AH arms: the ESP arm failed to whitelist OFPIEH12_FRAG,
while the AH arm accidentally whitelisted OFPIEH12_AUTH.

With the fix, a packet with two AH headers now also gets OFPIEH12_UNSEQ
in addition to OFPIEH12_UNREP, matching the ESP arm which already sets
UNSEQ on a repeat, which is the intended behavior.

Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Aaron Conole <aconole@redhat.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

---
v2 changes:
  - Updated commit message as requested by Sashiko.
---
 net/openvswitch/flow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 46c1d66aad8c..868d9fdf3afd 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -288,7 +288,7 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
 			if (*ext_hdrs & OFPIEH12_ESP)
 				*ext_hdrs |= OFPIEH12_UNREP;
 			if ((*ext_hdrs & ~(OFPIEH12_HOP | OFPIEH12_DEST |
-					   OFPIEH12_ROUTER | IPPROTO_FRAGMENT |
+					   OFPIEH12_ROUTER | OFPIEH12_FRAG |
 					   OFPIEH12_AUTH | OFPIEH12_UNREP)) ||
 			    dest_options_header_count >= 2) {
 				*ext_hdrs |= OFPIEH12_UNSEQ;
@@ -301,7 +301,7 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
 				*ext_hdrs |= OFPIEH12_UNREP;
 			if ((*ext_hdrs &
 			     ~(OFPIEH12_HOP | OFPIEH12_DEST | OFPIEH12_ROUTER |
-			       IPPROTO_FRAGMENT | OFPIEH12_UNREP)) ||
+			       OFPIEH12_FRAG | OFPIEH12_UNREP)) ||
 			    dest_options_header_count >= 2) {
 				*ext_hdrs |= OFPIEH12_UNSEQ;
 			}
-- 
2.54.0

Re: [PATCH net v2] openvswitch: fix wrong flag value 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: 0 (1) · Medium: 1 · Low: 0

- [Medium] The `case IPPROTO_ESP:` arm of get_ipv6_ext_hdrs()
  (net/openvswitch/flow.c:287-297), whose whitelist this patch changes…

Pre-existing issues:
- [High] In get_ipv6_ext_hdrs() the loop tail unconditionally advances
  the parse offset with `start += ipv6_optlen(hp)`…

--

openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()

This replaces two uses of IPPROTO_FRAGMENT with OFPIEH12_FRAG in the ESP
and AH arms of get_ipv6_ext_hdrs(), where the constant was being used as a
bitmask of already-seen extension headers.  The stated effect is that a
packet with two AH headers now reports OFPIEH12_UNSEQ in addition to
OFPIEH12_UNREP.

A couple of questions on the ESP arm and on the offset arithmetic in the
same loop.

> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 46c1d66aad8c..868d9fdf3afd 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -288,7 +288,7 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
>  			if (*ext_hdrs & OFPIEH12_ESP)
>  				*ext_hdrs |= OFPIEH12_UNREP;
>  			if ((*ext_hdrs & ~(OFPIEH12_HOP | OFPIEH12_DEST |
> -					   OFPIEH12_ROUTER | IPPROTO_FRAGMENT |
> +					   OFPIEH12_ROUTER | OFPIEH12_FRAG |
>  					   OFPIEH12_AUTH | OFPIEH12_UNREP)) ||
>  			    dest_options_header_count >= 2) {
>  				*ext_hdrs |= OFPIEH12_UNSEQ;

[Severity: Medium]
Can the case IPPROTO_ESP arm ever run?  The switch in get_ipv6_ext_hdrs()
is inside:

	while (ipv6_ext_hdr(next_type)) {

and ipv6_ext_hdr() in net/ipv6/exthdrs_core.c only accepts a fixed set of
next-header values:

	return   (nexthdr == NEXTHDR_HOP)	||
		 (nexthdr == NEXTHDR_ROUTING)	||
		 (nexthdr == NEXTHDR_FRAGMENT)	||
		 (nexthdr == NEXTHDR_AUTH)	||
		 (nexthdr == NEXTHDR_NONE)	||
		 (nexthdr == NEXTHDR_DEST);

NEXTHDR_ESP (50) is not in that list, so the loop exits before the ESP arm
is entered, and *ext_hdrs |= OFPIEH12_ESP at the end of that arm never
executes.  That would also mean OFPIEH12_ESP is never reported in
key->ipv6.exthdrs, and the OFPIEH12_ESP entry in the second-DSTOPTS
whitelist can never be exercised.

If that reading is right, is this part of the commit message accurate?

	This caused incorrect OFPIEH12_UNSEQ verdicts in
	both the ESP and AH arms: the ESP arm failed to whitelist OFPIEH12_FRAG,

Only the AH hunk seems able to change an observable verdict.

Should making the ESP arm reachable be part of this change, for example by
admitting ESP into the walk:

	while (ipv6_ext_hdr(next_type) || next_type == IPPROTO_ESP) {

Note that would also need a separate advance for ESP, since the ESP header
has no ipv6_opt_hdr-style hdrlen field.

> @@ -301,7 +301,7 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
>  				*ext_hdrs |= OFPIEH12_UNREP;
>  			if ((*ext_hdrs &
>  			     ~(OFPIEH12_HOP | OFPIEH12_DEST | OFPIEH12_ROUTER |
> -			       IPPROTO_FRAGMENT | OFPIEH12_UNREP)) ||
> +			       OFPIEH12_FRAG | OFPIEH12_UNREP)) ||
>  			    dest_options_header_count >= 2) {
>  				*ext_hdrs |= OFPIEH12_UNSEQ;
>  			}

[Severity: High]
This isn't a bug introduced by this patch, it dates back to 28a3f0601727,
but since the AH arm is being touched here: is the offset advance at the
bottom of the same loop in get_ipv6_ext_hdrs() correct for AH and Fragment
headers?

	hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
	if (!hp)
		break;
	next_type = hp->nexthdr;
	start += ipv6_optlen(hp);

ipv6_optlen() is only the right arithmetic for HOPOPTS/DSTOPTS/ROUTING.
>From include/linux/ipv6.h:

	#define ipv6_optlen(p)  (((p)->hdrlen+1) << 3)
	#define ipv6_authlen(p) (((p)->hdrlen+2) << 2)

For an AH with hdrlen=4 the header is 24 bytes, but ipv6_optlen() returns
40, so start lands 16 bytes inside the ICV/payload.  For a Fragment header,
the byte read as hdrlen is the Reserved field, which the sender controls and
receivers are required to ignore; the correct advance is a fixed 8.

The kernel's own chain walker handles both cases explicitly in
ipv6_find_hdr() (net/ipv6/exthdrs_core.c):

			hdrlen = 8;
		} else if (nexthdr == NEXTHDR_AUTH) {
			if (flags && (*flags & IP6_FH_F_AUTH) && (target < 0))
				break;
			hdrlen = ipv6_authlen(hp);
		} else
			hdrlen = ipv6_optlen(hp);

With the current code, any header parsed after an AH header, or after a
Fragment header with a non-zero Reserved byte, has its nexthdr and hdrlen
read from sender-chosen payload bytes, so the OFPIEH12_* bits stored in
key->ipv6.exthdrs and exported as OVS_KEY_ATTR_IPV6_EXTHDRS can be steered
remotely.  Payload bytes that look like a second AH or a HOPOPTS header
would raise OFPIEH12_UNREP/OFPIEH12_UNSEQ, and genuine trailing headers can
be missed.  There is no memory-safety concern here since
skb_header_pointer() is bounds-checked and start advances by at least 8 each
iteration.

The two-AH example given in the commit message still behaves as described,
since next_type comes from the previous header's correctly-read nexthdr
field.  Would it make sense to fix the per-type length handling in a
follow-up?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/1b1582eb07550d71f3cbe210e5cb31eeb8d0ad86.1788876917.git.echaudro%40redhat.com

Re: [PATCH net v2] openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  8 Sep 2026 16:15:17 +0200 you wrote:
> The ESP and AH cases in get_ipv6_ext_hdrs() used IPPROTO_FRAGMENT instead
> of OFPIEH12_FRAG when checking for out-of-order extension headers, causing
> the fragment header to not be recognised as a valid predecessor.
> 
> The original code used IPPROTO_FRAGMENT (44) as a bitmask constant where
> OFPIEH12_FRAG (1 << 4 = 16) was intended.  IPPROTO_FRAGMENT encodes bits
> 2, 3 and 5 (OFPIEH12_AUTH | OFPIEH12_DEST | OFPIEH12_ROUTER), but not
> bit 4 (OFPIEH12_FRAG).  This caused incorrect OFPIEH12_UNSEQ verdicts in
> both the ESP and AH arms: the ESP arm failed to whitelist OFPIEH12_FRAG,
> while the AH arm accidentally whitelisted OFPIEH12_AUTH.
> 
> [...]

Here is the summary with links:
  - [net,v2] openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()
    https://git.kernel.org/netdev/net/c/e184a4a6f423

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html