| Message ID | 2021110119033968552461@chinatelecom.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bug fix: avoid install bad dp flow | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 11/1/21 12:03, lic121 wrote: > dataofs field of tcp header indicates the tcp header len. The len > should be >= 20 bytes/4. This patch is to test dataofs, and don't > parse layer 4 fields when meet ba dataofs. This behave is the consistent > with openvswitch kenrel module. > > Signed-off-by: lic121 <lic121@chinatelecom.cn> > --- > lib/flow.c | 18 ++++++++++-------- > tests/ofproto-dpif.at | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 8 deletions(-) Hi. Thanks for the patch! And sorry for the late response. See some comments inline. Bets regards, Ilya Maximets. > > diff --git a/lib/flow.c b/lib/flow.c > index 89837de..f117490 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -1006,14 +1006,16 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) > if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) { > if (OVS_LIKELY(size >= TCP_HEADER_LEN)) { > const struct tcp_header *tcp = data; > - > - miniflow_push_be32(mf, arp_tha.ea[2], 0); > - miniflow_push_be32(mf, tcp_flags, > - TCP_FLAGS_BE32(tcp->tcp_ctl)); > - miniflow_push_be16(mf, tp_src, tcp->tcp_src); > - miniflow_push_be16(mf, tp_dst, tcp->tcp_dst); > - miniflow_push_be16(mf, ct_tp_src, ct_tp_src); > - miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); > + size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4; Please, add an empty line between the declarations and the code below. > + if (tcp_hdr_len >= TCP_HEADER_LEN) { This check seems fine, but, IIUC, it doesn't check the case where the TCP header length declared larger than the remaining space in a packet. Looking at the kernel implementation of tcphdr_ok(), there are, basically, 3 checks that make it fail: 1. pskb_may_pull(skb, th_ofs + sizeof(struct tcphdr))) == false This one is similar to (size >= TCP_HEADER_LEN) check here. 2. tcp_len < sizeof(struct tcphdr) This check you added as (tcp_hdr_len >= TCP_HEADER_LEN) 3. skb->len < th_ofs + tcp_len But this one is not covered by the patch. IIUC, it should look like: if (tcp_hdr_len >= TCP_HEADER_LEN && size >= tcp_hdr_len) { Without the third check, if TCP header length is larger than the remaining packet size, kernel will not parse it, but userspace will, leading to the similar issue. Could you add this check to the patch and a test case for this condition? > + miniflow_push_be32(mf, arp_tha.ea[2], 0); > + miniflow_push_be32(mf, tcp_flags, > + TCP_FLAGS_BE32(tcp->tcp_ctl)); Please, shift this line 3 spaces to the right. > + miniflow_push_be16(mf, tp_src, tcp->tcp_src); > + miniflow_push_be16(mf, tp_dst, tcp->tcp_dst); > + miniflow_push_be16(mf, ct_tp_src, ct_tp_src); > + miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); > + } > } > } else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) { > if (OVS_LIKELY(size >= UDP_HEADER_LEN)) { > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 31fb163..0f372ae 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -4862,6 +4862,37 @@ recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - malformed packets handling - upcall]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 90 > +dnl drop packet has tcp port 0-f but allow other tcp packets > +AT_DATA([flows.txt], [dnl > +priority=75 tcp tp_dst=0/0xfff0 actions=drop > +priority=50 tcp actions=output:1 > +]) > +AT_CHECK([ovs-ofctl replace-flows br0 flows.txt]) > +dnl good tcp pkt, tcp(sport=100,dpor=16) > +pkt1="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000005002200053330000" > +dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1) > +pkt2="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000001002200093330000" > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) > +mode=normal > +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt1"], [0], [stdout]) > +dnl for good tcp pkt, ovs can extract the tp_dst=16 > +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl > +flow-dump from the main thread: > +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=16/0xfff0), packets:0, bytes:0, used:never, actions:1 > +]) > +AT_CHECK([ovs-appctl dpctl/del-flows], [0], [stdout]) > +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt2"], [0], [stdout]) > +dnl for malformed tcp pkt, ovs can use default value tp_dst=0 > +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl > +flow-dump from the main thread: > +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0), packets:0, bytes:0, used:never, actions:drop > +]) > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - exit]) > OVS_VSWITCHD_START > add_of_ports br0 1 2 3 10 11 12 13 14 >
>On 11/1/21 12:03, lic121 wrote: >> dataofs field of tcp header indicates the tcp header len. The len >> should be >= 20 bytes/4. This patch is to test dataofs, and don't >> parse layer 4 fields when meet ba dataofs. This behave is the consistent >> with openvswitch kenrel module. >> >> Signed-off-by: lic121 <lic121@chinatelecom.cn> >> --- >> lib/flow.c | 18 ++++++++++-------- >> tests/ofproto-dpif.at | 31 +++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+), 8 deletions(-) > >Hi. Thanks for the patch! And sorry for the late response. >See some comments inline. > Hi Ilya, it's OK and thanks for your review. >Bets regards, Ilya Maximets. > >> >> diff --git a/lib/flow.c b/lib/flow.c >> index 89837de..f117490 100644 >> --- a/lib/flow.c >> +++ b/lib/flow.c >> @@ -1006,14 +1006,16 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) >> if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) { >> if (OVS_LIKELY(size >= TCP_HEADER_LEN)) { >> const struct tcp_header *tcp = data; >> - >> - miniflow_push_be32(mf, arp_tha.ea[2], 0); >> - miniflow_push_be32(mf, tcp_flags, >> - TCP_FLAGS_BE32(tcp->tcp_ctl)); >> - miniflow_push_be16(mf, tp_src, tcp->tcp_src); >> - miniflow_push_be16(mf, tp_dst, tcp->tcp_dst); >> - miniflow_push_be16(mf, ct_tp_src, ct_tp_src); >> - miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); >> + size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4; > >Please, add an empty line between the declarations and the code below. Will address this in v4 > >> + if (tcp_hdr_len >= TCP_HEADER_LEN) { > >This check seems fine, but, IIUC, it doesn't check the case where >the TCP header length declared larger than the remaining space in >a packet. Looking at the kernel implementation of tcphdr_ok(), >there are, basically, 3 checks that make it fail: > >1. pskb_may_pull(skb, th_ofs + sizeof(struct tcphdr))) == false > > This one is similar to (size >= TCP_HEADER_LEN) check here. > >2. tcp_len < sizeof(struct tcphdr) > > This check you added as (tcp_hdr_len >= TCP_HEADER_LEN) > >3. skb->len < th_ofs + tcp_len > > But this one is not covered by the patch. IIUC, it should > look like: > if (tcp_hdr_len >= TCP_HEADER_LEN && size >= tcp_hdr_len) { > >Without the third check, if TCP header length is larger than >the remaining packet size, kernel will not parse it, but >userspace will, leading to the similar issue. > >Could you add this check to the patch and a test case for this >condition? Agree, will address this in v4 > >> + miniflow_push_be32(mf, arp_tha.ea[2], 0); >> + miniflow_push_be32(mf, tcp_flags, >> + TCP_FLAGS_BE32(tcp->tcp_ctl)); > >Please, shift this line 3 spaces to the right. good catch. Will do > >> + miniflow_push_be16(mf, tp_src, tcp->tcp_src); >> + miniflow_push_be16(mf, tp_dst, tcp->tcp_dst); >> + miniflow_push_be16(mf, ct_tp_src, ct_tp_src); >> + miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); >> + } >> } >> } else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) { >> if (OVS_LIKELY(size >= UDP_HEADER_LEN)) { >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 31fb163..0f372ae 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -4862,6 +4862,37 @@ recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ofproto-dpif - malformed packets handling - upcall]) >> +OVS_VSWITCHD_START >> +add_of_ports br0 1 90 >> +dnl drop packet has tcp port 0-f but allow other tcp packets >> +AT_DATA([flows.txt], [dnl >> +priority=75 tcp tp_dst=0/0xfff0 actions=drop >> +priority=50 tcp actions=output:1 >> +]) >> +AT_CHECK([ovs-ofctl replace-flows br0 flows.txt]) >> +dnl good tcp pkt, tcp(sport=100,dpor=16) >> +pkt1="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000005002200053330000" >> +dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1) >> +pkt2="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000001002200093330000" >> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) >> +mode=normal >> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt1"], [0], [stdout]) >> +dnl for good tcp pkt, ovs can extract the tp_dst=16 >> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl >> +flow-dump from the main thread: >> +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=16/0xfff0), packets:0, bytes:0, used:never, actions:1 >> +]) >> +AT_CHECK([ovs-appctl dpctl/del-flows], [0], [stdout]) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt2"], [0], [stdout]) >> +dnl for malformed tcp pkt, ovs can use default value tp_dst=0 >> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl >> +flow-dump from the main thread: >> +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0), packets:0, bytes:0, used:never, actions:drop >> +]) >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([ofproto-dpif - exit]) >> OVS_VSWITCHD_START >> add_of_ports br0 1 2 3 10 11 12 13 14 >> > >
diff --git a/lib/flow.c b/lib/flow.c index 89837de..f117490 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -1006,14 +1006,16 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) { if (OVS_LIKELY(size >= TCP_HEADER_LEN)) { const struct tcp_header *tcp = data; - - miniflow_push_be32(mf, arp_tha.ea[2], 0); - miniflow_push_be32(mf, tcp_flags, - TCP_FLAGS_BE32(tcp->tcp_ctl)); - miniflow_push_be16(mf, tp_src, tcp->tcp_src); - miniflow_push_be16(mf, tp_dst, tcp->tcp_dst); - miniflow_push_be16(mf, ct_tp_src, ct_tp_src); - miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); + size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4; + if (tcp_hdr_len >= TCP_HEADER_LEN) { + miniflow_push_be32(mf, arp_tha.ea[2], 0); + miniflow_push_be32(mf, tcp_flags, + TCP_FLAGS_BE32(tcp->tcp_ctl)); + miniflow_push_be16(mf, tp_src, tcp->tcp_src); + miniflow_push_be16(mf, tp_dst, tcp->tcp_dst); + miniflow_push_be16(mf, ct_tp_src, ct_tp_src); + miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); + } } } else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) { if (OVS_LIKELY(size >= UDP_HEADER_LEN)) { diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 31fb163..0f372ae 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -4862,6 +4862,37 @@ recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - malformed packets handling - upcall]) +OVS_VSWITCHD_START +add_of_ports br0 1 90 +dnl drop packet has tcp port 0-f but allow other tcp packets +AT_DATA([flows.txt], [dnl +priority=75 tcp tp_dst=0/0xfff0 actions=drop +priority=50 tcp actions=output:1 +]) +AT_CHECK([ovs-ofctl replace-flows br0 flows.txt]) +dnl good tcp pkt, tcp(sport=100,dpor=16) +pkt1="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000005002200053330000" +dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1) +pkt2="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000001002200093330000" +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) +mode=normal +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt1"], [0], [stdout]) +dnl for good tcp pkt, ovs can extract the tp_dst=16 +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl +flow-dump from the main thread: +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=16/0xfff0), packets:0, bytes:0, used:never, actions:1 +]) +AT_CHECK([ovs-appctl dpctl/del-flows], [0], [stdout]) +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt2"], [0], [stdout]) +dnl for malformed tcp pkt, ovs can use default value tp_dst=0 +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl +flow-dump from the main thread: +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0), packets:0, bytes:0, used:never, actions:drop +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - exit]) OVS_VSWITCHD_START add_of_ports br0 1 2 3 10 11 12 13 14
dataofs field of tcp header indicates the tcp header len. The len should be >= 20 bytes/4. This patch is to test dataofs, and don't parse layer 4 fields when meet ba dataofs. This behave is the consistent with openvswitch kenrel module. Signed-off-by: lic121 <lic121@chinatelecom.cn> --- lib/flow.c | 18 ++++++++++-------- tests/ofproto-dpif.at | 31 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-)