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

[netdev-bot+sashiko@kernel.org]

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