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

[Ilya Maximets <i.maximets@ovn.org>]

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.