On 8/9/26 3:03 PM, Eli Britstein wrote:
> 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/ipf.c | 46 ++++++++++++++++++++++++++++++++-----------
> tests/ofproto-dpif.at | 45 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 11 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index d836b8824..274c02f84 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -434,7 +434,7 @@ ipf_reassemble_v4_frags(struct ipf_list *ipf_list)
> int rest_len = frag_list[ipf_list->last_inuse_idx].end_data_byte -
> frag_list[1].start_data_byte + 1;
>
> - if (orig_len + rest_len > IPV4_PACKET_MAX_SIZE) {
> + if (rest_len <= 0 || orig_len + rest_len > IPV4_PACKET_MAX_SIZE) {
> ipf_print_reass_packet(
> "Unsupported big reassembled v4 packet; v4 hdr:", l3);
This verdict is not accurate when rest_len <= 0. We should have a
separate one.
> dp_packet_delete(pkt);
> @@ -484,7 +484,7 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list)
> int rest_len = frag_list[ipf_list->last_inuse_idx].end_data_byte -
> frag_list[1].start_data_byte + 1;
>
> - if (orig_len + rest_len > IPV6_PACKET_MAX_DATA) {
> + if (rest_len <= 0 || orig_len + rest_len > IPV6_PACKET_MAX_DATA) {
> ipf_print_reass_packet(
> "Unsupported big reassembled v6 packet; v6 hdr:", l3);
Same here.
> dp_packet_delete(pkt);
> @@ -668,9 +668,17 @@ ipf_v4_key_extract(struct dp_packet *pkt, ovs_be16
> dl_type, uint16_t zone,
> 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;
>
> - *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;
> + /* 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) {
> + return false;
> + }
> +
> + *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);
> @@ -742,7 +750,7 @@ invalid_pkt:
>
> }
>
> -static void
> +static bool
> ipf_v6_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)
> @@ -758,9 +766,18 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16
> dl_type, uint16_t zone,
> NULL);
> ovs_assert(nw_frag && frag_hdr);
> ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg;
> - *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) +
> + uint32_t start = ntohs(ip6f_offlg & IP6F_OFF_MASK) +
> sizeof (struct ovs_16aligned_ip6_frag);
> - *end_data_byte = *start_data_byte + dp_packet_l4_size(pkt) - 1;
> + size_t l4_size = dp_packet_l4_size(pkt);
> +
> + /* As in ipf_v4_key_extract(), reject fragments that would wrap the
> + * uint16_t fragment bounds. */
> + if (!l4_size || start + l4_size - 1 > UINT16_MAX) {
> + return false;
> + }
> +
> + *start_data_byte = start;
> + *end_data_byte = start + l4_size - 1;
> *ff = ipf_is_first_v6_frag(ip6f_offlg);
> *lf = ipf_is_last_v6_frag(ip6f_offlg);
> memset(key, 0, sizeof *key);
> @@ -773,6 +790,7 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16
> dl_type, uint16_t zone,
> key->nw_proto = 0; /* Not used for key for V6. */
> key->zone = zone;
> key->recirc_id = pkt->md.recirc_id;
> + return true;
> }
>
> static bool
> @@ -892,11 +910,17 @@ ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt,
> ovs_be16 dl_type,
> bool v6 = dl_type == htons(ETH_TYPE_IPV6);
>
> if (v6 && ipf_get_v6_enabled(ipf)) {
> - ipf_v6_key_extract(pkt, dl_type, zone, &key, &start_data_byte,
> - &end_data_byte, &ff, &lf);
> + if (!ipf_v6_key_extract(pkt, dl_type, zone, &key, &start_data_byte,
> + &end_data_byte, &ff, &lf)) {
> + pkt->md.ct_state = CS_INVALID;
> + return false;
> + }
> } else if (!v6 && ipf_get_v4_enabled(ipf)) {
> - ipf_v4_key_extract(pkt, dl_type, zone, &key, &start_data_byte,
> - &end_data_byte, &ff, &lf);
> + if (!ipf_v4_key_extract(pkt, dl_type, zone, &key, &start_data_byte,
> + &end_data_byte, &ff, &lf)) {
> + pkt->md.ct_state = CS_INVALID;
> + return false;
> + }
> } else {
> return false;
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 644b7b7fb..7e231b967 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5671,6 +5671,51 @@ 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 001. .... = Flags: 0x1, More fragments
> +dnl 0... .... = Reserved bit: Not set
> +dnl .0.. .... = Don't fragment: Not set
> +dnl ..1. .... = More fragments: Set
The packet below doesn't have the "More fragments" bit 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
Maybe good to wrap this line. It's a little too long.
> + num frag: 0
> + v4 frags accepted: 0
> + v6 frags accepted: 0
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto-dpif - handling of malformed TCP packets])
> OVS_VSWITCHD_START
> add_of_ports br0 1 90
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
