I think it's a problem which causes an unexpected megaflow. The flow maybe a drop megaflow to drop the normal packet. You can see it in the test.
The ovs kernel datapath does not check the length field when it extracts L3 info from the skb. The parser process is different between the kernel datapath(ovs_flow_key_extract) and the userspace(miniflow_extract/ipv4_sanity_check) No better method to handle the invalid packet, 1) if the kernel datapath drops the invalid packet, how about L2 switch? 2) if the kernel datapath parses only to L2 info like the userpace does, the drop megaflow(no L3) will drop the valid L3 packet. So this fix does as the kernel datapath does. From: Flavio Leitner <[email protected]> Date: 2023-03-27 23:26:27 To: Simon Horman <[email protected]> Cc: Faicker Mo <[email protected]>,[email protected],Ilya Maximets <[email protected]> Subject: Re: [ovs-dev] [PATCH] flow: fix sanity check for unexpected ip header length field>On Mon, Mar 27, 2023 at 03:34:52PM +0200, Simon Horman wrote: >> On Wed, Mar 15, 2023 at 05:11:01PM +0800, Faicker Mo wrote: >> > Derivation cases of CVE-2020-35498: >> > 1. invalid ipv4 header total-length field >> > 2. invalid ipv6 header payload-length field >> > These may cause unwanted flow to send to datapath. >> > >> > >> > Signed-off-by: Faicker Mo <[email protected]> >> >> I think the immediate question here is how to correctly handle invalid >> packets which claim to have a total length that is greater than the length >> received (size) by OVS. > >OVS strives to forward all packets but at the same time it >can't rely on data from non-conforming packets. > >You can see with checksum that when OVS changes a packet >it recalculates the checksum in a way that it doesn't >change the original state (good/bad). > >> It doesn't seem to me that truncating the total length to size, >> and thus pretending the packets are valid in this regard, >> is the right approach. > >+1 > >> Is there a bigger problem in that any packets that fail the sanity check >> are problematic with regards to megaflow creation? If so, I think we should >> aim for a more comprehensive solution. > >+1 > >fbl > > >> >> > --- >> > lib/flow.c | 11 +++++------ >> > tests/classifier.at | 42 ++++++++++++++++++++++++++++++++++++++++++ >> > tests/ofp-print.at | 2 +- >> > 3 files changed, 48 insertions(+), 7 deletions(-) >> > >> > >> > diff --git a/lib/flow.c b/lib/flow.c >> > index c3a3aa3ce..d96d02213 100644 >> > --- a/lib/flow.c >> > +++ b/lib/flow.c >> > @@ -662,9 +662,8 @@ ipv4_sanity_check(const struct ip_header *nh, size_t >> > size, >> > return false; >> > } >> > >> > - tot_len = ntohs(nh->ip_tot_len); >> > - if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len || >> > - size - tot_len > UINT16_MAX)) { >> > + tot_len = MIN(size, ntohs(nh->ip_tot_len)); >> > + if (OVS_UNLIKELY(ip_len > tot_len || size - tot_len > UINT16_MAX)) { >> > COVERAGE_INC(miniflow_extract_ipv4_pkt_len_error); >> > return false; >> > } >> > @@ -700,7 +699,7 @@ ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr >> > *nh, size_t size) >> > return false; >> > } >> > >> > - plen = ntohs(nh->ip6_plen); >> > + plen = MIN(size - sizeof *nh, ntohs(nh->ip6_plen)); >> > if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) { >> > COVERAGE_INC(miniflow_extract_ipv6_pkt_len_error); >> > return false; >> > @@ -920,7 +919,7 @@ miniflow_extract(struct dp_packet *packet, struct >> > miniflow *dst) >> > } >> > data_pull(&data, &size, sizeof *nh); >> > >> > - plen = ntohs(nh->ip6_plen); >> > + plen = MIN(size, ntohs(nh->ip6_plen)); >> > dp_packet_set_l2_pad_size(packet, size - plen); >> > size = plen; /* Never pull padding. */ >> > >> > @@ -1197,7 +1196,7 @@ parse_tcp_flags(struct dp_packet *packet, >> > } >> > data_pull(&data, &size, sizeof *nh); >> > >> > - plen = ntohs(nh->ip6_plen); /* Never pull padding. */ >> > + plen = MIN(size, ntohs(nh->ip6_plen)); /* Never pull padding. */ >> > dp_packet_set_l2_pad_size(packet, size - plen); >> > size = plen; >> > const struct ovs_16aligned_ip6_frag *frag_hdr; >> > diff --git a/tests/classifier.at b/tests/classifier.at >> > index de2705653..1a1615bb5 100644 >> > --- a/tests/classifier.at >> > +++ b/tests/classifier.at >> > @@ -418,6 +418,7 @@ ovs-ofctl: "conjunction" actions may be used along >> > with "note" but not any other >> > OVS_VSWITCHD_STOP >> > AT_CLEANUP >> > >> > +AT_BANNER([flow classifier abnormal packet]) >> > # Flow classifier a packet with excess of padding. >> > AT_SETUP([flow classifier - packet with extra padding]) >> > OVS_VSWITCHD_START >> > @@ -453,3 +454,44 @@ Datapath actions: 2 >> > ]) >> > OVS_VSWITCHD_STOP >> > AT_CLEANUP >> > + >> > +dnl Flow classifier a packet with invalid total-length field of ipv4 >> > header >> > +AT_SETUP([flow classifier - packet with invalid total-length field of >> > ipv4 header]) >> > +OVS_VSWITCHD_START >> > +add_of_ports br0 1 2 >> > +AT_DATA([flows.txt], [dnl >> > +priority=5,ip,ip_dst=1.1.1.1,actions=1 >> > +priority=5,ip,ip_dst=1.1.1.2,actions=2 >> > +priority=0,actions=drop >> > +]) >> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> > +packet=00000000000000000000000008004500003c00010000401176ac0101010101010102003500350008fb4f >> > +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=1 $packet] , [0], [stdout]) >> > +dnl the problem flow, >> > +dnl Megaflow: recirc_id=0,eth,ip,in_port=1,nw_dst=0.0.0.0/8,nw_frag=no >> > +dnl Datapath actions: drop >> > +AT_CHECK([tail -2 stdout], [0], >> > + [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_dst=1.1.1.2,nw_frag=no >> > +Datapath actions: 2 >> > +]) >> > +OVS_VSWITCHD_STOP >> > +AT_CLEANUP >> > + >> > +dnl Flow classifier a packet with invalid payload-length field of ipv4 >> > header >> > +AT_SETUP([flow classifier - packet with invalid payload-length field of >> > ipv6 header]) >> > +OVS_VSWITCHD_START >> > +add_of_ports br0 1 2 >> > +AT_DATA([flows.txt], [dnl >> > +priority=5,ipv6,ipv6_dst=1::1,actions=1 >> > +priority=5,ipv6,ipv6_dst=1::2,actions=2 >> > +priority=0,actions=drop >> > +]) >> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> > +packet=00000000000000000000000086dd60000000001e11400001000000000000000000000000000100010000000000000000000000000002003500350008ff6f >> > +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=1 $packet] , [0], [stdout]) >> > +AT_CHECK([tail -2 stdout], [0], >> > + [Megaflow: recirc_id=0,eth,ipv6,in_port=1,ipv6_dst=1::2,nw_frag=no >> > +Datapath actions: 2 >> > +]) >> > +OVS_VSWITCHD_STOP >> > +AT_CLEANUP >> > diff --git a/tests/ofp-print.at b/tests/ofp-print.at >> > index 14aa55416..0c786a542 100644 >> > --- a/tests/ofp-print.at >> > +++ b/tests/ofp-print.at >> > @@ -3151,7 +3151,7 @@ AT_CHECK([ovs-ofctl ofp-print " >> > ], [0], [dnl >> > NXT_PACKET_IN2 (xid=0x0): table_id=7 cookie=0xfedcba9876543210 >> > total_len=64 metadata=0x5a5a5a5a5a5a5a5a (via action) data_len=48 >> > buffer=0x00000114 >> > userdata=01.02.03.04.05 >> > -ip,dl_vlan=80,dl_vlan_pcp=0,vlan_tci1=0x0000,dl_src=80:81:81:81:81:81,dl_dst=82:82:82:82:82:82,nw_src=0.0.0.0,nw_dst=0.0.0.0,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no >> > +tcp,dl_vlan=80,dl_vlan_pcp=0,vlan_tci1=0x0000,dl_src=80:81:81:81:81:81,dl_dst=82:82:82:82:82:82,nw_src=83.83.83.83,nw_dst=84.84.84.84,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no,tp_src=0,tp_dst=0,tcp_flags=0 >> > ]) >> > AT_CLEANUP >> > >> > -- >> > 2.31.1 >> > >> > >> > >> > >> > >> > >> > >> > >> > _______________________________________________ >> > dev mailing list >> > [email protected] >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >-- >fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
