netstacklat: Add sanity check for out-of-order sequence

The logic for excluding samples from TCP reads that may have been
delayed by HOL blocking relies on reading a number of fields from the
TCP socket outside of the socket lock. This may be prone to errors due
to the socket state being updated at another place in the kernel while
our eBPF program is running. To reduce the risk that a data race
causes the filter to fail, add a sanity check for the maximum out of
order sequence used to exclude future TCP reads from monitoring.

The most problematic of the read fields in the tcp_sock is
ooo_last_skb, as that is a pointer to another SKB rather than a direct
value. This pointer is only valid as long as the out_of_order_queue is
non-empty. Due to a data race, we may check that the ooo-queue is
non-empty while there are still SKBs in it, then have the kernel clear
out the ooo-queue, and finally attempt to read the ooo_last_skb
pointer later when it is no longer valid (and may now point to a
freed/recycled SKB). This may result in incorrect values being used
for the sequence limit used to exclude future reads of
ooo-segments. The faulty sequence limit may both cause reads of
HOL-blocked segments to be included or the exclusion of an
unnecessarily large amount of future reads (up to 2 GB).

To reduce the risk that the garbage data from an invalid SKB is used,
introduce two sanity checks for end_seq in the ooo_last_skb. First
check if the sequence number is zero, if so assume it is invalid (even
though it can be a valid sequence number). Even though we will get an
error code if reading the data from this SKB fails altogether, we may
still succeed reading from a no longer valid SKB, in which case there
is a high risk the data will have been zeroed. If it's non-zero, also
check that it is within the current receive window (if not, clamp it
to the receive window).

Signed-off-by: Simon Sundberg <[email protected]>
Signed-off-by: Jesper Dangaard Brouer <[email protected]>

Author: simosund

examples/netstacklat.bpf.c

@@ -504,36 +504,85 @@ static __always_inline bool filter_socket(struct sock *sk, struct sk_buff *skb,
 }
 #endif
 
+/* Get the current receive window end sequence for tp
+ * In the kernel receive window checks are done against
+ * tp->rcv_nxt + tcp_receive_window(tp). This function should give a compareable
+ * result, i.e. rcv_wup + rcv_wnd or rcv_nxt, whichever is higher
+ */
+static int get_current_rcv_wnd_seq(struct tcp_sock *tp, u32 rcv_nxt, u32 *seq)
+{
+	u32 rcv_wup, rcv_wnd, window = 0;
+	int err;
+
+	err = bpf_core_read(&rcv_wup, sizeof(rcv_wup), &tp->rcv_wup);
+	if (err) {
+		bpf_printk("failed to read tcp_sock->rcv_wup, err=%d", err);
+		goto exit;
+	}
+
+	err = bpf_core_read(&rcv_wnd, sizeof(rcv_wnd), &tp->rcv_wnd);
+	if (err) {
+		bpf_printk("failed to read tcp_sock->rcv_wnd, err=%d", err);
+		goto exit;
+	}
+
+	window = rcv_wup + rcv_wnd;
+	if (u32_lt(window, rcv_nxt))
+		window = rcv_nxt;
+
+exit:
+	*seq = window;
+	return err;
+}
+
 static int current_max_possible_ooo_seq(struct tcp_sock *tp, u32 *seq)
 {
+	u32 rcv_nxt, cur_rcv_window, max_seq = 0;
 	struct tcp_skb_cb cb;
-	u32 max_seq = 0;
 	int err = 0;
 
+	err = bpf_core_read(&rcv_nxt, sizeof(rcv_nxt), &tp->rcv_nxt);
+	if (err) {
+		bpf_printk("failed reading tcp_sock->rcv_nxt, err=%d", err);
+		goto exit;
+	}
+
 	if (BPF_CORE_READ(tp, out_of_order_queue.rb_node) == NULL) {
 		/* No ooo-segments currently in ooo-queue
 		 * Any ooo-segments must already have been merged to the
 		 * receive queue. Current rcv_nxt must therefore be ahead
 		 * of all ooo-segments that have arrived until now.
 		 */
-		err = bpf_core_read(&max_seq, sizeof(max_seq), &tp->rcv_nxt);
-		if (err)
-			bpf_printk("failed to read tcp_sock->rcv_nxt, err=%d",
-				   err);
+		max_seq = rcv_nxt;
 	} else {
 		/*
 		 * Some ooo-segments currently in ooo-queue
 		 * Max out-of-order seq is given by the seq_end of the tail
 		 * skb in the ooo-queue.
 		 */
 		err = BPF_CORE_READ_INTO(&cb, tp, ooo_last_skb, cb);
-		if (err)
+		if (err) {
 			bpf_printk(
 				"failed to read tcp_sock->ooo_last_skb->cb, err=%d",
 				err);
-		max_seq = cb.end_seq;
+			goto exit;
+		}
+
+		// Sanity check - ooo_last_skb->cb.end_seq within the receive window?
+		err = get_current_rcv_wnd_seq(tp, rcv_nxt, &cur_rcv_window);
+		if (err)
+			goto exit;
+
+		/* While seq 0 can be a valid seq, consider it more likely to
+		 * be the result of reading from an invalid SKB pointer
+		 */
+		if (cb.end_seq == 0 || u32_lt(cur_rcv_window, cb.end_seq))
+			max_seq = cur_rcv_window;
+		else
+			max_seq = cb.end_seq;
 	}
 
+exit:
 	*seq = max_seq;
 	return err;
 }