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(-)
diff --git a/lib/dpctl.c b/lib/dpctl.c
index d6ff348e5..48afb8549 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 purged: %"PRIu64"\n",
@@ -2640,6 +2642,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 purged: %"PRIu64"\n",
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 402e765d2..b3dd58e9d 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_purged;
unsigned int min_frag_size;
diff --git a/lib/ipf.c b/lib/ipf.c
index 185d6432e..ff7881235 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_PURGED,
IPF_NFRAGS_NUM_CNTS,
@@ -434,6 +435,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);
@@ -484,6 +492,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);
@@ -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);
+ 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);
@@ -742,10 +767,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;
@@ -758,9 +784,19 @@ 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) {
+ 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);
@@ -773,6 +809,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 +929,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;
}
@@ -1439,6 +1482,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_PURGED],
@@ -1455,6 +1500,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_PURGED],
diff --git a/lib/ipf.h b/lib/ipf.h
index 7324a1704..2ac3c9658 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_purged;
unsigned int min_frag_size;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 6de6f293f..1c117f5b4 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -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
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - handling of malformed TCP packets])
OVS_VSWITCHD_START
add_of_ports br0 1 90
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index f0d9121e3..10ce3e746 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 purged: 0
min v6 frag size: 1280
@@ -185,6 +186,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 purged: 0
])
@@ -208,6 +210,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 purged: 0
min v6 frag size: 1280
@@ -215,6 +218,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 purged: 0
@@ -241,6 +245,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 purged: 0
min v6 frag size: 1280
@@ -248,6 +253,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 purged: 0
@@ -281,6 +287,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 purged: 0
min v6 frag size: 1280
@@ -288,6 +295,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 purged: 0
--
2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
