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

[PATCH net] 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.

Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 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] openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()

[Ilya Maximets]

On 9/3/26 10:18 AM, Eelco Chaudron 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.
> 
> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")

Cc: stable@vger.kernel.org

> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>

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

[Aaron Conole]

Eelco Chaudron <echaudro@redhat.com> writes:

> 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.
>
> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net] 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 (2) · Medium: 1 · Low: 0

- [Medium] Undisclosed second behavioural change in the IPPROTO_AH arm of
  get_ipv6_ext_hdrs() (net/openvswitch/flow.c:299-309).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the `case
  IPPROTO_ESP:` arm of get_ipv6_ext_hdrs()…
- [High] This problem wasn't introduced by this patch, but
  get_ipv6_ext_hdrs() advances the parse offset with `start +=…

--

openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()

The ESP and AH arms of get_ipv6_ext_hdrs() masked out the numeric constant
IPPROTO_FRAGMENT instead of the OFPIEH12_FRAG bit when deciding whether the
extension headers seen so far are out of order.  Both masks are changed to
use OFPIEH12_FRAG.

A few questions below about the two hunks and the surrounding parser.

> 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: High]
This isn't a bug introduced by this patch, but is the "case IPPROTO_ESP:"
arm being fixed here reachable at all?

The parse loop in get_ipv6_ext_hdrs() is gated on:

	while (ipv6_ext_hdr(next_type)) {

and ipv6_ext_hdr() in net/ipv6/exthdrs_core.c only accepts:

	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 when the header walk reaches an
ESP header the loop exits before the switch is entered.

Does that mean OFPIEH12_ESP is never OR-ed into key->ipv6.exthdrs, and
therefore the bit exported to userspace through OVS_KEY_ATTR_IPV6_EXTHDRS
(struct ovs_key_ipv6_exthdrs.hdrs) is always zero, so a flow matching
OFPIEH12_ESP=1 can never match?

If so, the ESP repeat test above, and the OFPIEH12_ESP term in the second
Destination Options mask, would also be dead, and the kernel-doc for
get_ipv6_ext_hdrs() which lists the "Encapsulating Security Payload header"
in the recommended ordering would describe behaviour the code cannot
produce.  Would handling ESP as an explicitly admitted terminal header,
rather than relying on the generic ipv6_ext_hdr() gate, be the right
direction here?

> @@ -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: Medium]
Does this hunk also change the reported flags for packets carrying two
Authentication Headers?

>From net/openvswitch/flow.h:

	OFPIEH12_ESP    = 1 << 1,
	OFPIEH12_AUTH   = 1 << 2,
	OFPIEH12_DEST   = 1 << 3,
	OFPIEH12_FRAG   = 1 << 4,
	OFPIEH12_ROUTER = 1 << 5,

IPPROTO_FRAGMENT is 44, which is 32 | 8 | 4, i.e. OFPIEH12_ROUTER |
OFPIEH12_DEST | OFPIEH12_AUTH.  So the old mask here happened to whitelist
OFPIEH12_AUTH as well, while the new mask (HOP | DEST | ROUTER | FRAG |
UNREP) does not.

A packet with two AH headers previously got only OFPIEH12_UNREP set, and
now gets OFPIEH12_UNSEQ in addition, which changes the userspace-visible
OVS_KEY_ATTR_IPV6_EXTHDRS value.  The new behaviour matches the ESP arm,
which already sets UNSEQ on a repeat, so it may well be intended, but since
this carries a Fixes: tag and is a stable candidate, could the commit
message mention this second change instead of only the fragment header
predecessor case?

[Severity: High]
This is a pre-existing issue, but the AH ordering verdict computed here
depends on the chain walk that follows it being correct, and the tail of the
loop in get_ipv6_ext_hdrs() advances with:

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

Is ipv6_optlen() correct for an Authentication Header?  include/linux/ipv6.h
has:

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

and ipv6_skip_exthdr() in net/ipv6/exthdrs_core.c picks the latter:

	} else if (nexthdr == NEXTHDR_AUTH)
		hdrlen = ipv6_authlen(hp);
	else
		hdrlen = ipv6_optlen(hp);

For a common AH with a 96-bit ICV (hdrlen == 4) this walker advances 40
bytes instead of 24, so the next skb_header_pointer() reads ICV or payload
bytes chosen by the sender.

That value becomes next_type and drives all further advancement.  Can a
crafted AH packet therefore hide real headers that follow the AH (a second
Destination Options, Fragment, ESP), or fabricate OFPIEH12_FRAG /
OFPIEH12_ROUTER / OFPIEH12_DEST / OFPIEH12_UNSEQ / OFPIEH12_UNREP bits out
of payload data, letting flows keyed on the extension-header pseudo-field be
evaded?

Should this use ipv6_authlen() when next_type is IPPROTO_AH, the way the
core stack does?

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

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

[Eelco Chaudron]

On 8 Sep 2026, at 10:21, netdev-bot+sashiko@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0
>
> - [Medium] Undisclosed second behavioural change in the IPPROTO_AH arm of
>   get_ipv6_ext_hdrs() (net/openvswitch/flow.c:299-309).

For this I sent a new revision with an updated commit message.

> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the `case
>   IPPROTO_ESP:` arm of get_ipv6_ext_hdrs()…
> - [High] This problem wasn't introduced by this patch, but
>   get_ipv6_ext_hdrs() advances the parse offset with `start +=…

I'll add an item to my todo list, to take a look at those at a later time.

> --
>
> openvswitch: fix wrong flag value in get_ipv6_ext_hdrs()
>
> The ESP and AH arms of get_ipv6_ext_hdrs() masked out the numeric constant
> IPPROTO_FRAGMENT instead of the OFPIEH12_FRAG bit when deciding whether the
> extension headers seen so far are out of order.  Both masks are changed to
> use OFPIEH12_FRAG.
>
> A few questions below about the two hunks and the surrounding parser.
>
>> 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: High]
> This isn't a bug introduced by this patch, but is the "case IPPROTO_ESP:"
> arm being fixed here reachable at all?
>
> The parse loop in get_ipv6_ext_hdrs() is gated on:
>
> 	while (ipv6_ext_hdr(next_type)) {
>
> and ipv6_ext_hdr() in net/ipv6/exthdrs_core.c only accepts:
>
> 	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 when the header walk reaches an
> ESP header the loop exits before the switch is entered.
>
> Does that mean OFPIEH12_ESP is never OR-ed into key->ipv6.exthdrs, and
> therefore the bit exported to userspace through OVS_KEY_ATTR_IPV6_EXTHDRS
> (struct ovs_key_ipv6_exthdrs.hdrs) is always zero, so a flow matching
> OFPIEH12_ESP=1 can never match?
>
> If so, the ESP repeat test above, and the OFPIEH12_ESP term in the second
> Destination Options mask, would also be dead, and the kernel-doc for
> get_ipv6_ext_hdrs() which lists the "Encapsulating Security Payload header"
> in the recommended ordering would describe behaviour the code cannot
> produce.  Would handling ESP as an explicitly admitted terminal header,
> rather than relying on the generic ipv6_ext_hdr() gate, be the right
> direction here?
>
>> @@ -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: Medium]
> Does this hunk also change the reported flags for packets carrying two
> Authentication Headers?
>
>> From net/openvswitch/flow.h:
>
> 	OFPIEH12_ESP    = 1 << 1,
> 	OFPIEH12_AUTH   = 1 << 2,
> 	OFPIEH12_DEST   = 1 << 3,
> 	OFPIEH12_FRAG   = 1 << 4,
> 	OFPIEH12_ROUTER = 1 << 5,
>
> IPPROTO_FRAGMENT is 44, which is 32 | 8 | 4, i.e. OFPIEH12_ROUTER |
> OFPIEH12_DEST | OFPIEH12_AUTH.  So the old mask here happened to whitelist
> OFPIEH12_AUTH as well, while the new mask (HOP | DEST | ROUTER | FRAG |
> UNREP) does not.
>
> A packet with two AH headers previously got only OFPIEH12_UNREP set, and
> now gets OFPIEH12_UNSEQ in addition, which changes the userspace-visible
> OVS_KEY_ATTR_IPV6_EXTHDRS value.  The new behaviour matches the ESP arm,
> which already sets UNSEQ on a repeat, so it may well be intended, but since
> this carries a Fixes: tag and is a stable candidate, could the commit
> message mention this second change instead of only the fragment header
> predecessor case?
>
> [Severity: High]
> This is a pre-existing issue, but the AH ordering verdict computed here
> depends on the chain walk that follows it being correct, and the tail of the
> loop in get_ipv6_ext_hdrs() advances with:
>
> 	hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
> 	if (!hp)
> 		break;
> 	next_type = hp->nexthdr;
> 	start += ipv6_optlen(hp);
>
> Is ipv6_optlen() correct for an Authentication Header?  include/linux/ipv6.h
> has:
>
> 	#define ipv6_optlen(p)  (((p)->hdrlen+1) << 3)
> 	#define ipv6_authlen(p) (((p)->hdrlen+2) << 2)
>
> and ipv6_skip_exthdr() in net/ipv6/exthdrs_core.c picks the latter:
>
> 	} else if (nexthdr == NEXTHDR_AUTH)
> 		hdrlen = ipv6_authlen(hp);
> 	else
> 		hdrlen = ipv6_optlen(hp);
>
> For a common AH with a 96-bit ICV (hdrlen == 4) this walker advances 40
> bytes instead of 24, so the next skb_header_pointer() reads ICV or payload
> bytes chosen by the sender.
>
> That value becomes next_type and drives all further advancement.  Can a
> crafted AH packet therefore hide real headers that follow the AH (a second
> Destination Options, Fragment, ESP), or fabricate OFPIEH12_FRAG /
> OFPIEH12_ROUTER / OFPIEH12_DEST / OFPIEH12_UNSEQ / OFPIEH12_UNREP bits out
> of payload data, letting flows keyed on the extension-header pseudo-field be
> evaded?
>
> Should this use ipv6_authlen() when next_type is IPPROTO_AH, the way the
> core stack does?
>
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/d42d6f04596dacab6cabfb1f06aaf4bd53394d3d.1788423539.git.echaudro%40redhat.com