On 8 Sep 2026, at 10:21, [email protected] 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
