[ovs-dev] [PATCH v3 1/3] upcall: prevent from installing flows when inconsistence

lic121 lic121 at chinatelecom.cn
Mon Nov 1 11:02:32 UTC 2021


In ovs kernel datapath upcall, the *key* and packet are passed to
userspace. The key contains the fields/meta extracted from packet.
Once the ovs-vswitchd receives the upcall, the packet is extracted
again into *flow*. Next, the flow is used to match openflow rules to
generate the wildcard(wc). At last, vswitchd installs a mega_flow in
datapath(mega_flow = key/wc,action)

We can see that vswitchd generate wc from flow while it installs dp
flow with key. If the key is not consistent with the flow [1], we get
bad mega_flow.

Let's assume we have the flowing rules, means to block tcp port 0-0xf,
but allow other ports.

"table=0,priority=100,tcp,tp_dst=0x0/0xfff0 actions=drop"
"table=0,priority=90,tcp actions=p1"

good case:
If a packet has tcp dst=0x10, generated `mega_flow=0x10/0xfff0,out:p1`,
this is expected.

bad case:
If a packet has tcp dst=0x10 but not pass tcphdr_ok [1], generated wc
and action are `0xfff0,out:p1`. The mega_flow will be
`0x0/0xfff0,out:p1`, bacause mega_flow=key/wc,action. This allows
packets with tcp port 0-0xf pass by mistake.

The following scapy3 script triggers the issue:
```py
eth=Ether(src="fa:16:3e:5e:e3:57",dst="be:95:df:40:fb:57")
ip=IP(src="10.10.10.10",dst="20.20.20.20")
tcp=TCP(sport=100,dport=16,dataofs=1)
sendp(eth/ip/tcp)
```

This patch is to prevent from installing datapath flow if the key is
not consistant with the flow.

[1] https://github.com/openvswitch/ovs/blob/v2.16.1/datapath/flow.c#L601

Signed-off-by: lic121 <lic121 at chinatelecom.cn>
---
 ofproto/ofproto-dpif-upcall.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 1c9c720..93c750d 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -244,6 +244,7 @@ struct upcall {
     size_t key_len;                /* Datapath flow key length. */
     const struct nlattr *out_tun_key;  /* Datapath output tunnel key. */
 
+    const struct flow *key_as_flow;       /* converted from key. */
     struct user_action_cookie cookie;
 
     uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */
@@ -810,6 +811,7 @@ recv_upcalls(struct handler *handler)
     struct dpif_upcall dupcalls[UPCALL_MAX_BATCH];
     struct upcall upcalls[UPCALL_MAX_BATCH];
     struct flow flows[UPCALL_MAX_BATCH];
+    struct flow key_as_flows[UPCALL_MAX_BATCH];
     size_t n_upcalls, i;
 
     n_upcalls = 0;
@@ -818,6 +820,7 @@ recv_upcalls(struct handler *handler)
         struct dpif_upcall *dupcall = &dupcalls[n_upcalls];
         struct upcall *upcall = &upcalls[n_upcalls];
         struct flow *flow = &flows[n_upcalls];
+        struct flow *key_as_flow = &key_as_flows[n_upcalls];
         unsigned int mru = 0;
         uint64_t hash = 0;
         int error;
@@ -830,7 +833,7 @@ recv_upcalls(struct handler *handler)
         }
 
         upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len,
-                                               flow, NULL);
+                                               key_as_flow, NULL);
         if (upcall->fitness == ODP_FIT_ERROR) {
             goto free_dupcall;
         }
@@ -842,7 +845,9 @@ recv_upcalls(struct handler *handler)
         if (dupcall->hash) {
             hash = nl_attr_get_u64(dupcall->hash);
         }
-
+        /* Fill flow with key_as_flow as upcall_receive needs
+         * packet flow info. */
+        *flow = *key_as_flow;
         error = upcall_receive(upcall, udpif->backer, &dupcall->packet,
                                dupcall->type, dupcall->userdata, flow, mru,
                                &dupcall->ufid, PMD_ID_NULL);
@@ -856,20 +861,21 @@ recv_upcalls(struct handler *handler)
                               dupcall->key_len, NULL, 0, NULL, 0,
                               &dupcall->ufid, PMD_ID_NULL, NULL);
                 VLOG_INFO_RL(&rl, "received packet on unassociated datapath "
-                             "port %"PRIu32, flow->in_port.odp_port);
+                             "port %"PRIu32, key_as_flow->in_port.odp_port);
             }
             goto free_dupcall;
         }
 
         upcall->key = dupcall->key;
         upcall->key_len = dupcall->key_len;
+        upcall->key_as_flow = key_as_flow;
         upcall->ufid = &dupcall->ufid;
         upcall->hash = hash;
 
         upcall->out_tun_key = dupcall->out_tun_key;
         upcall->actions = dupcall->actions;
 
-        pkt_metadata_from_flow(&dupcall->packet.md, flow);
+        pkt_metadata_from_flow(&dupcall->packet.md, key_as_flow);
         flow_extract(&dupcall->packet, flow);
 
         error = process_upcall(udpif, upcall,
@@ -1332,6 +1338,21 @@ should_install_flow(struct udpif *udpif, struct upcall *upcall)
         return false;
     }
 
+    /* For linux kernel datapath, the "key" extracted by kernel may be
+    inconsistent with the flow extracted from packet by ovs. If that
+    is the case, twe can't install the datapth flow (key/wc) */
+    if (upcall->key_len) {
+        if (!flow_equal_except(upcall->key_as_flow, upcall->flow,
+                               &upcall->wc)) {
+            VLOG_INFO_RL(&rl, "upcall: inconsistent on datapath key and "
+                         "vswitchd extracted key. Datapath flow will not be "
+                         "installed\n"
+                         "datapath key: %s \nvswitchd extracted key: %s",
+                         flow_to_string(upcall->key_as_flow, NULL),
+                         flow_to_string(upcall->flow, NULL));
+            return false;
+        }
+    }
     return true;
 }
 
-- 
1.8.3.1



More information about the dev mailing list