diff mbox series

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

Message ID 2021112123205523936749@chinatelecom.cn
State Deferred
Headers show
Series bug fix: avoid install bad dp flow | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Cheng Li Nov. 21, 2021, 3:20 p.m. UTC 
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@chinatelecom.cn>
---
 ofproto/ofproto-dpif-upcall.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Comments

Simon Horman Oct. 13, 2023, 9:04 a.m. UTC | #1 
On Sun, Nov 21, 2021 at 11:20:55PM +0800, lic121 wrote:
> 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@chinatelecom.cn>

Hi,

This patch appears to have gone stale in patchwork, for one reason or
another. If it is still relevant then I think it needs to be revisited,
by being reposted after appropriate preparation.

As such I'm marking this patch as "Deferred" in patchwork.

No action is required unless there is a desire to revisit this patch.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 1c9c720..81f297d 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;
         }
@@ -843,6 +846,9 @@  recv_upcalls(struct handler *handler)
             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 +862,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 +1339,19 @@  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 && !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;
 }