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