From 2cfbcd5247ed0fd941c1ebb9f4adb952b67fe13a Mon Sep 17 00:00:00 2001 From: Timothy Redaelli Date: Thu, 23 Nov 2023 19:47:54 +0100 Subject: [PATCH] netdev-offload-tc: Check geneve metadata length. Currently ovs-vswitchd crashes, with hw offloading enabled, if a geneve packet with corrupted metadata is received, because the metadata header is not verified correctly. This commit adds a check for geneve metadata length and, if the header is wrong, the packet is not sent to flower. It also includes a system-traffic test for geneve packets with corrupted metadata. Fixes: a468645c6d33 ("lib/tc: add geneve with option match offload") Reported-by: Haresh Khandelwal Signed-off-by: Timothy Redaelli Signed-off-by: Ilya Maximets --- lib/netdev-offload-tc.c | 25 ++++++++++++++++++++----- tests/system-traffic.at | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 164c7eef63e..921d5231777 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1785,12 +1785,12 @@ test_key_and_mask(struct match *match) return 0; } -static void +static int flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, struct flow_tnl *tnl_mask) { struct geneve_opt *opt, *opt_mask; - int len, cnt = 0; + int tot_opt_len, len, cnt = 0; /* 'flower' always has an exact match on tunnel metadata length, so having * it in a wrong format is not acceptable unless it is empty. */ @@ -1806,7 +1806,7 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, memset(&tnl_mask->metadata.present.map, 0, sizeof tnl_mask->metadata.present.map); } - return; + return 0; } tnl_mask->flags &= ~FLOW_TNL_F_UDPIF; @@ -1820,7 +1820,7 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, sizeof tnl_mask->metadata.present.len); if (!tnl->metadata.present.len) { - return; + return 0; } memcpy(flower->key.tunnel.metadata.opts.gnv, tnl->metadata.opts.gnv, @@ -1834,7 +1834,16 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, * also not masks, but actual lengths in the 'flower' structure. */ len = flower->key.tunnel.metadata.present.len; while (len) { + if (len < sizeof *opt) { + return EOPNOTSUPP; + } + opt = &flower->key.tunnel.metadata.opts.gnv[cnt]; + tot_opt_len = sizeof *opt + opt->length * 4; + if (len < tot_opt_len) { + return EOPNOTSUPP; + } + opt_mask = &flower->mask.tunnel.metadata.opts.gnv[cnt]; opt_mask->length = opt->length; @@ -1842,6 +1851,8 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, cnt += sizeof(struct geneve_opt) / 4 + opt->length; len -= sizeof(struct geneve_opt) + opt->length * 4; } + + return 0; } static void @@ -2287,7 +2298,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); if (!strcmp(netdev_get_type(netdev), "geneve")) { - flower_match_to_tun_opt(&flower, tnl, tnl_mask); + err = flower_match_to_tun_opt(&flower, tnl, tnl_mask); + if (err) { + VLOG_WARN_RL(&warn_rl, "Unable to parse geneve options"); + return err; + } } flower.tunnel = true; } else { diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 418cd32..4c40580 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -903,6 +903,39 @@ ovs-pcap p0.pcap AT_CHECK([ovs-pcap p0.pcap | grep -Eq "^[[[:xdigit:]]]{24}86dd60000000003a1140fc000000000000000000000000000100fc000000000000000000000000000001[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}0000655800000000fffffffffffffa163e949d8008060001080006040001[[[:xdigit:]]]{12}0a0000f40000000000000a0000fe$"]) AT_CLEANUP +AT_SETUP([datapath - handling of geneve corrupted metadata]) +OVS_CHECK_TUNNEL_TSO() +OVS_CHECK_GENEVE() + +OVS_TRAFFIC_VSWITCHD_START( + [_ADD_BR([br-underlay]) -- \ + set bridge br0 other-config:hwaddr=f2:ff:00:00:00:01 -- \ + set bridge br-underlay other-config:hwaddr=f2:ff:00:00:00:02]) + +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) + +ADD_NAMESPACES(at_ns0) + +dnl Set up underlay link from host into the namespace using veth pair. +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24", f2:ff:00:00:00:03) +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) +AT_CHECK([ip link set dev br-underlay up]) + +dnl Set up tunnel endpoints on OVS outside the namespace and with a native +dnl linux device inside the namespace. +ADD_OVS_TUNNEL([geneve], [br0], [at_gnv0], [172.31.1.1], [10.1.1.100/24]) +ADD_NATIVE_TUNNEL([geneve], [ns_gnv0], [at_ns0], [172.31.1.100], [10.1.1.1/24], + [vni 0], [address f2:ff:00:00:00:04]) + +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f2 ff 00 00 00 02 f2 ff 00 00 00 03 08 00 45 00 00 52 00 01 00 00 40 11 1f f7 ac 1f 01 01 ac 1f 01 64 de c1 17 c1 00 3e 59 e9 01 00 65 58 00 00 00 00 00 03 00 02 f2 ff 00 00 00 01 f2 ff 00 00 00 04 08 00 45 00 00 1c 00 01 00 00 40 01 64 7a 0a 01 01 01 0a 01 01 64 08 00 f7 ff 00 00 00 00 > /dev/null]) + +OVS_WAIT_UNTIL([grep -q 'Invalid Geneve tunnel metadata' ovs-vswitchd.log]) + +OVS_TRAFFIC_VSWITCHD_STOP(["/Invalid Geneve tunnel metadata on bridge br0 while processing icmp,in_port=1,vlan_tci=0x0000,dl_src=f2:ff:00:00:00:04,dl_dst=f2:ff:00:00:00:01,nw_src=10.1.1.1,nw_dst=10.1.1.100,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=8,icmp_code=0/d +/Unable to parse geneve options/d"]) +AT_CLEANUP + AT_SETUP([datapath - ping over gre tunnel by simulated packets]) OVS_CHECK_TUNNEL_TSO() OVS_CHECK_MIN_KERNEL(3, 10)