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.
RFC 791 permits zero-payload last fragments, but common stacks including
the Linux kernel discard them, and they now fail this check too. Align
the reassembly test with that policy by using valid one-byte fragments.
[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 | 90 ++++++++++++++++++++++++++------
lib/ipf.h | 1 +
tests/ofproto-dpif.at | 55 +++++++++++++++++--
tests/system-userspace-macros.at | 8 +++
6 files changed, 140 insertions(+), 19 deletions(-)
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 6732a90c0..dc3b68c01 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -2625,6 +2625,8 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
dpif_ipf_status.v4.nfrag_expired_sent);
dpctl_print(dpctl_p, " v4 frags too small: %"PRIu64"\n",
dpif_ipf_status.v4.nfrag_too_small);
+ dpctl_print(dpctl_p, " v4 frags too large: %"PRIu64"\n",
+ dpif_ipf_status.v4.nfrag_too_large);
dpctl_print(dpctl_p, " v4 frags overlapped: %"PRIu64"\n",
dpif_ipf_status.v4.nfrag_overlap);
dpctl_print(dpctl_p, " v4 frags duplicate: %"PRIu64"\n",
@@ -2644,6 +2646,8 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
dpif_ipf_status.v6.nfrag_expired_sent);
dpctl_print(dpctl_p, " v6 frags too small: %"PRIu64"\n",
dpif_ipf_status.v6.nfrag_too_small);
+ dpctl_print(dpctl_p, " v6 frags too large: %"PRIu64"\n",
+ dpif_ipf_status.v6.nfrag_too_large);
dpctl_print(dpctl_p, " v6 frags overlapped: %"PRIu64"\n",
dpif_ipf_status.v6.nfrag_overlap);
dpctl_print(dpctl_p, " v6 frags duplicate: %"PRIu64"\n",
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index eb80fb68d..f418327df 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -129,6 +129,7 @@ struct dpif_ipf_proto_status {
uint64_t nfrag_completed_sent;
uint64_t nfrag_expired_sent;
uint64_t nfrag_too_small;
+ uint64_t nfrag_too_large;
uint64_t nfrag_overlap;
uint64_t nfrag_duplicate;
uint64_t nfrag_discarded;
diff --git a/lib/ipf.c b/lib/ipf.c
index 95ef07f49..87ff5b4b9 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -82,6 +82,7 @@ enum ipf_counter_type {
IPF_NFRAGS_COMPL_SENT,
IPF_NFRAGS_EXPIRED,
IPF_NFRAGS_TOO_SMALL,
+ IPF_NFRAGS_TOO_LARGE,
IPF_NFRAGS_OVERLAP,
IPF_NFRAGS_DUPLICATE,
IPF_NFRAGS_DISCARDED,
@@ -436,6 +437,13 @@ 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 (rest_len <= 0) {
+ ipf_print_reass_packet(
+ "Invalid v4 fragment bounds in reassembly; v4 hdr:", l3);
+ dp_packet_delete(pkt);
+ return NULL;
+ }
+
if (orig_len + rest_len > IPV4_PACKET_MAX_SIZE) {
ipf_print_reass_packet(
"Unsupported big reassembled v4 packet; v4 hdr:", l3);
@@ -486,6 +494,13 @@ 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 (rest_len <= 0) {
+ ipf_print_reass_packet(
+ "Invalid v6 fragment bounds in reassembly; v6 hdr:", l3);
+ dp_packet_delete(pkt);
+ return NULL;
+ }
+
if (orig_len + rest_len > IPV6_PACKET_MAX_DATA) {
ipf_print_reass_packet(
"Unsupported big reassembled v6 packet; v6 hdr:", l3);
@@ -663,16 +678,32 @@ 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;
+
+ /* A fragment carrying no data is degenerate and cannot contribute to
+ * reassembly. */
+ if (ip_tot_len <= ip_hdr_len) {
+ ipf_count(ipf, false, IPF_NFRAGS_TOO_SMALL);
+ return false;
+ }
+
+ /* Fragments whose data ends past 65535 bytes would wrap the uint16_t
+ * fragment bounds, corrupting reassembly. */
+ if (start + (ip_tot_len - ip_hdr_len) - 1 > UINT16_MAX) {
+ ipf_count(ipf, false, IPF_NFRAGS_TOO_LARGE);
+ 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);
@@ -744,10 +775,11 @@ invalid_pkt:
}
-static void
-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)
+static bool
+ipf_v6_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 ovs_16aligned_ip6_hdr *l3 = dp_packet_l3(pkt);
uint8_t nw_frag = 0;
@@ -760,9 +792,26 @@ 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);
+
+ /* A fragment carrying no data is degenerate and cannot contribute to
+ * reassembly. */
+ if (!l4_size) {
+ ipf_count(ipf, true, IPF_NFRAGS_TOO_SMALL);
+ return false;
+ }
+
+ /* As in ipf_v4_key_extract(), reject fragments whose data would wrap the
+ * uint16_t fragment bounds. */
+ if (start + l4_size - 1 > UINT16_MAX) {
+ ipf_count(ipf, true, IPF_NFRAGS_TOO_LARGE);
+ 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);
@@ -775,6 +824,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
@@ -940,11 +990,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(ipf, pkt, dl_type, zone, &key,
+ &start_data_byte, &end_data_byte, &ff, &lf)) {
+ dp_packet_delete(pkt);
+ return true;
+ }
} 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(ipf, pkt, dl_type, zone, &key,
+ &start_data_byte, &end_data_byte, &ff, &lf)) {
+ dp_packet_delete(pkt);
+ return true;
+ }
} else {
return false;
}
@@ -1487,6 +1543,8 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
*ipf_status)
&ipf_status->v4.nfrag_expired_sent);
atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_TOO_SMALL],
&ipf_status->v4.nfrag_too_small);
+ atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_TOO_LARGE],
+ &ipf_status->v4.nfrag_too_large);
atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_OVERLAP],
&ipf_status->v4.nfrag_overlap);
atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_DUPLICATE],
@@ -1507,6 +1565,8 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
*ipf_status)
&ipf_status->v6.nfrag_expired_sent);
atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_TOO_SMALL],
&ipf_status->v6.nfrag_too_small);
+ atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_TOO_LARGE],
+ &ipf_status->v6.nfrag_too_large);
atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_OVERLAP],
&ipf_status->v6.nfrag_overlap);
atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_DUPLICATE],
diff --git a/lib/ipf.h b/lib/ipf.h
index f7ad5d77e..c8aec5a5a 100644
--- a/lib/ipf.h
+++ b/lib/ipf.h
@@ -27,6 +27,7 @@ struct ipf_proto_status {
uint64_t nfrag_completed_sent;
uint64_t nfrag_expired_sent;
uint64_t nfrag_too_small;
+ uint64_t nfrag_too_large;
uint64_t nfrag_overlap;
uint64_t nfrag_duplicate;
uint64_t nfrag_discarded;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index d968f5633..04ac3f446 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5536,9 +5536,9 @@ table=2 tcp actions=3
AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt])
dnl Test frag expiry.
-AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0015 0001 8192 40 06 3315 ac11370d ac11370b 00"])
ovs-appctl time/warp 10000
-AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0015 0001 8192 40 06 3315 ac11370d ac11370b 00"])
dnl Test that no packets flow.
AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | strip_recirc |
sort], [0], [dnl
@@ -5550,9 +5550,9 @@ dnl Expire second frag.
ovs-appctl time/warp 10000
dnl Test frag purge
-AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0014 0002 8192 40 06 3315 ac11370d ac11370b"])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0015 0002 8192 40 06 3314 ac11370d ac11370b 00"])
ovs-appctl time/warp 33000
-AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0014 0003 8192 40 06 3314 ac11370d ac11370b"])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9 0800
4500 0015 0003 8192 40 06 3313 ac11370d ac11370b 00"])
dnl Test that no packets flow.
AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | strip_recirc |
sort], [0], [dnl
@@ -5728,6 +5728,53 @@ AT_CHECK([test 0 = `ovs-ofctl parse-pcap p1-tx.pcap | wc
-l`])
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:|v4 frags too large:'], [], [dnl
+ num frag: 0
+ v4 frags accepted: 0
+ v4 frags too large: 1
+ v6 frags accepted: 0
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - fragment handling - reject overlapped fragment])
OVS_VSWITCHD_START
add_of_ports br0 1 90
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index 07e50bbdc..1358d7156 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -178,6 +178,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
v4 frags completed: 0
v4 frags expired: 0
v4 frags too small: 0
+ v4 frags too large: 0
v4 frags overlapped: 0
v4 frags duplicate: 0
v4 frags discarded: 0
@@ -187,6 +188,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
v6 frags completed: 0
v6 frags expired: 0
v6 frags too small: 0
+ v6 frags too large: 0
v6 frags overlapped: 0
v6 frags duplicate: 0
v6 frags discarded: 0
@@ -212,6 +214,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v4 frags completed: 30
v4 frags expired: 0
v4 frags too small: 0
+ v4 frags too large: 0
v4 frags overlapped: 0
v4 frags duplicate: 0
v4 frags discarded: 0
@@ -221,6 +224,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v6 frags completed: 0
v6 frags expired: 0
v6 frags too small: 0
+ v6 frags too large: 0
v6 frags overlapped: 0
v6 frags duplicate: 0
v6 frags discarded: 0
@@ -249,6 +253,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v4 frags completed: 0
v4 frags expired: 0
v4 frags too small: 0
+ v4 frags too large: 0
v4 frags overlapped: 0
v4 frags duplicate: 0
v4 frags discarded: 0
@@ -258,6 +263,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v6 frags completed: 30
v6 frags expired: 0
v6 frags too small: 0
+ v6 frags too large: 0
v6 frags overlapped: 0
v6 frags duplicate: 0
v6 frags discarded: 0
@@ -293,6 +299,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m |
FORMAT_FRAG_LIST()], [], [dnl
v4 frags completed: 0
v4 frags expired: 0
v4 frags too small: 0
+ v4 frags too large: 0
v4 frags overlapped: 0
v4 frags duplicate: 0
v4 frags discarded: 0
@@ -302,6 +309,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m |
FORMAT_FRAG_LIST()], [], [dnl
v6 frags completed: 0
v6 frags expired: 0
v6 frags too small: 0
+ v6 frags too large: 0
v6 frags overlapped: 0
v6 frags duplicate: 0
v6 frags discarded: 0
--
2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
