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/[email protected]/
> Cc: [email protected]
> Signed-off-by: Joas Antonio dos Santos <[email protected]>
> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
