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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
