Eli Britstein <[email protected]> writes:
> Commit [1] fixed the reassembly size cap to use dp_packet_size() rather
> than the IPv4 total-length or IPv6 payload-length of the first fragment.
> Those L3 fields describe only that fragment and exclude the Ethernet
> header, so they understated the buffer size being reassembled into.
>
> That check still uses uint16_t start/end offsets per fragment. If a
> fragment's data ends beyond byte 65535, the end offset wraps and the
> stored bounds are wrong. rest_len can become negative while
> orig_len + rest_len still fits under the cap, so reassembly proceeds on
> a corrupt fragment list.
>
> The wrapped bounds can still satisfy ipf_list_complete(). The negative
> rest_len is then passed as size_t to dp_packet_prealloc_tailroom(), and
> add_len wraps to a huge size_t in dp_packet_put(), causing out-of-
> bounds reads and writes on the reassembly buffer and heap corruption.
>
> Reject such fragments when extracting keys and fail reassembly when
> rest_len is not positive.
>
> [1] c3f4d9fe54f4 ("ipf: Fix the over-sized reassembly.")
>
> Assisted-by: composer-2.5-fast, Cursor
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Eli Britstein <[email protected]>
> ---
> lib/dpctl.c | 4 ++
> lib/dpif-provider.h | 1 +
> lib/ipf.c | 77 +++++++++++++++++++++++++-------
> lib/ipf.h | 1 +
> tests/ofproto-dpif.at | 46 +++++++++++++++++++
> tests/system-userspace-macros.at | 8 ++++
> 6 files changed, 122 insertions(+), 15 deletions(-)
[...]
> @@ -661,16 +676,26 @@ invalid_pkt:
> }
>
> static bool
> -ipf_v4_key_extract(struct dp_packet *pkt, ovs_be16 dl_type, uint16_t zone,
> - struct ipf_list_key *key, uint16_t *start_data_byte,
> - uint16_t *end_data_byte, bool *ff, bool *lf)
> +ipf_v4_key_extract(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type,
> + uint16_t zone, struct ipf_list_key *key,
> + uint16_t *start_data_byte, uint16_t *end_data_byte,
> + bool *ff, bool *lf)
> {
> const struct ip_header *l3 = dp_packet_l3(pkt);
> uint16_t ip_tot_len = ntohs(l3->ip_tot_len);
> size_t ip_hdr_len = IP_IHL(l3->ip_ihl_ver) * 4;
> + uint32_t start = ntohs(l3->ip_frag_off & htons(IP_FRAG_OFF_MASK)) * 8;
> +
> + /* Fragments with no data or with data ending past 65535 bytes would
> + * wrap the uint16_t fragment bounds, corrupting reassembly. */
> + if (ip_tot_len <= ip_hdr_len
> + || start + (ip_tot_len - ip_hdr_len) - 1 > UINT16_MAX) {
> + ipf_count(ipf, true, IPF_NFRAGS_TOO_LARGE);
^ should be false, I think.
> + return false;
> + }
>
> - *start_data_byte = ntohs(l3->ip_frag_off & htons(IP_FRAG_OFF_MASK)) * 8;
> - *end_data_byte = *start_data_byte + ip_tot_len - ip_hdr_len - 1;
> + *start_data_byte = start;
> + *end_data_byte = start + (ip_tot_len - ip_hdr_len) - 1;
> *ff = ipf_is_first_v4_frag(pkt);
> *lf = ipf_is_last_v4_frag(pkt);
> memset(key, 0, sizeof *key);
[...]
> @@ -5670,6 +5670,52 @@ CHECK_COVERAGE([dpif_netdev_output_grow_queues], [1])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - fragment handling - reject oversized fragment])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 90
> +
> +AT_DATA([flows.txt], [dnl
> +table=0 in_port=90,ip actions=ct(commit),output:1
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt])
> +
> +dnl Last fragment at offset 65528 carrying 28 data bytes: its end offset
> +dnl (65555) exceeds the maximum packet size and, truncated to 16 bits,
> +dnl wraps to 19. It must not be admitted for reassembly.
> +dnl IPv4 Packet content:
> +dnl Ethernet II, Src: 50:54:00:00:00:09, Dst: 50:54:00:00:00:0a
> +dnl Type: IPv4 (0x0800)
> +dnl Internet Protocol Version 4, Src: 10.1.1.1, Dst: 10.1.1.2
> +dnl 0100 .... = Version: 4
> +dnl .... 0101 = Header Length: 20 bytes (5)
> +dnl Differentiated Services Field: 0x00 (DSCP: CS0, ECN: Not-ECT)
> +dnl Total Length: 48
> +dnl Identification: 0x0001 (1)
> +dnl 000. .... = Flags: 0x0
> +dnl 0... .... = Reserved bit: Not set
> +dnl .0.. .... = Don't fragment: Not set
> +dnl ..0. .... = More fragments: Not set
> +dnl ...1 1111 1111 1111 = Fragment Offset: 65528
> +dnl Time to Live: 64
> +dnl Protocol: UDP (17)
> +dnl Header Checksum: 0x44b9
> +dnl Data (28 bytes)
> +eth="50 54 00 00 00 0a 50 54 00 00 00 09 08 00"
> +ip="45 00 00 30 00 01 1f ff 40 11 44 b9 0a 01 01 01 0a 01 01 02"
> +data="00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16
> 17 18 19 1a 1b"
> +packet="${eth} ${ip} ${data}"
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet"])
> +
> +AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
> +| grep -E 'num frag:|frags accepted:'], [], [dnl
> + num frag: 0
> + v4 frags accepted: 0
> + v6 frags accepted: 0
> +])
May also be worth asserting on the v4 frags too large here as well.
That probably would have flagged the 'true' argument to ipf_count in the
patch.
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
