[PATCH v3] bpf: bpf_dbg: split pcap_next_pkt() validation/advance, fix off-by-one in cmd_select

[Hasan Basbunar]

bpf_dbg's interactive 'select <N>' command, documented in the file
header ("select 3 (run etc will start from the 3rd packet in the
pcap)") to use 1-based packet indexing, advances the pcap cursor one
packet too many. The loop in cmd_select():

	pcap_reset_pkt();         /* cursor on packet 1 */
	for (i = 0; i < which && (have_next = pcap_next_pkt()); i++)
		/* noop */;

calls pcap_next_pkt() N times to reach packet N, but pcap_next_pkt()
validates the packet at the cursor and then advances past it. After
N calls the cursor is on packet N+1, so 'select 3' positions on
packet 4, 'select 4' on packet 5, etc.

Simply changing the loop init to 'i = 1' (so it advances N-1 times)
fixes the user-visible symptom but leaves the final landed-on packet
unvalidated, and combined with pcap_next_pkt()'s '>=' boundary
checks, mis-handles the boundary cases on the last and just-past-the-
last packet. As pointed out by the Sashiko AI review on v1 and v2,
this surfaces in two ways:

  1. On a perfect pcap (no trailing bytes after the last packet),
     pcap_next_pkt()'s '>= pcap_map_size' rejects packets whose body
     ends exactly at the file boundary, so 'select N' on an N-packet
     file errors as "no packet #N available" even though the packet
     is fully in-bounds.

  2. On a truncated pcap (filehdr + a few stray bytes that happen to
     pass try_load_pcap()'s 'pcap_map_size > sizeof(filehdr)' guard
     but not enough to contain a full pkthdr), 'select 1' returns
     CMD_OK without ever validating the header, and a subsequent
     'step' or 'run' dereferences pcap_curr_pkt()->caplen past the
     mapped region.

Fix all three issues by splitting pcap_next_pkt() into a pure
validator (pcap_curr_pkt_valid()) and a validate-advance-validate
combinator. The boundary check now uses '>' instead of '>=', so a
packet whose body ends exactly at pcap_map_size is correctly accepted.
pcap_next_pkt() returns true only when both the current packet was
valid and, after advancing, the new cursor position is also valid.
This means the do-while in cmd_run() exits cleanly after the last
packet (no past-end dereference), and cmd_select() can call
pcap_curr_pkt_valid() after the loop to bounds-check the final
packet.

Reproduction (deterministic, no kernel needed): build bpf_dbg from
the unmodified tree, synthesize a pcap with N>=2 packets each with a
distinct payload byte, and drive 'select 1 / step 1 / quit'. Before
this fix, 'select 1' shows packet 2's payload. After this fix,
'select K' shows packet K for all K in 1..N, 'select N+1' correctly
errors with "no packet #N+1 available!", and 'select 1' on a pcap
truncated to filehdr + 1 byte also correctly errors.

Cloudflare's downstream mirror at github.com/cloudflare/bpftools
carries the same defect.

Fixes: fd981e3c321a ("filter: bpf_dbg: add minimal bpf debugger")
Signed-off-by: Hasan Basbunar <basbunarhasan@xxxxxxxxx>
---
Changes in v3:
 - Split pcap_next_pkt() into pcap_curr_pkt_valid() (pure validator)
   and pcap_next_pkt() (validate-current, advance, validate-new).
 - Boundary check now uses '>' instead of '>='; a packet whose body
   ends exactly at pcap_map_size is correctly accepted.
 - cmd_select() validates the final landed-on packet via
   pcap_curr_pkt_valid() instead of the dead
   `pcap_curr_pkt() == NULL` check.
 - Empirically verified in a clean Debian container (gcc -Wall -O0)
   against:
     * 5-packet pcap, select K for K in 1..6 (5 successes + 1 error
       on K=6, payload byte matches K per the file header docs);
     * 1-packet pcap, select 1 (succeeds), select 2 (errors);
     * truncated pcap (filehdr + 1 byte), select 1 errors cleanly
       without dereferencing past the mapped region;
     * `run` after `select 3` on a 5-packet pcap processes exactly
       3 packets and exits cleanly without past-end deref.
 - Addresses both review concerns raised by Sashiko AI on v1 and v2.
 - v1: https://lore.kernel.org/bpf/20260428100109.56572-1-basbunarhasan@xxxxxxxxx/
   v2: https://lore.kernel.org/bpf/20260429084441.22089-1-basbunarhasan@xxxxxxxxx/

 tools/bpf/bpf_dbg.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/bpf_dbg.c b/tools/bpf/bpf_dbg.c
index 4895602ab37d..db12d2f8fb73 100644
--- a/tools/bpf/bpf_dbg.c
+++ b/tools/bpf/bpf_dbg.c
@@ -918,21 +918,30 @@ static struct pcap_pkthdr *pcap_curr_pkt(void)
 	return (void *) pcap_ptr_va_curr;
 }
 
-static bool pcap_next_pkt(void)
+static bool pcap_curr_pkt_valid(void)
 {
 	struct pcap_pkthdr *hdr = pcap_curr_pkt();
 
 	if (pcap_ptr_va_curr + sizeof(*hdr) -
-	    pcap_ptr_va_start >= pcap_map_size)
+	    pcap_ptr_va_start > pcap_map_size)
 		return false;
 	if (hdr->caplen == 0 || hdr->len == 0 || hdr->caplen > hdr->len)
 		return false;
 	if (pcap_ptr_va_curr + sizeof(*hdr) + hdr->caplen -
-	    pcap_ptr_va_start >= pcap_map_size)
+	    pcap_ptr_va_start > pcap_map_size)
 		return false;
+	return true;
+}
+
+static bool pcap_next_pkt(void)
+{
+	struct pcap_pkthdr *hdr;
 
+	if (!pcap_curr_pkt_valid())
+		return false;
+	hdr = pcap_curr_pkt();
 	pcap_ptr_va_curr += (sizeof(*hdr) + hdr->caplen);
-	return true;
+	return pcap_curr_pkt_valid();
 }
 
 static void pcap_reset_pkt(void)
@@ -1143,7 +1152,7 @@ static int cmd_select(char *num)
 
 	for (i = 1; i < which && (have_next = pcap_next_pkt()); i++)
 		/* noop */;
-	if (!have_next || pcap_curr_pkt() == NULL) {
+	if (!have_next || !pcap_curr_pkt_valid()) {
 		rl_printf("no packet #%u available!\n", which);
 		pcap_reset_pkt();
 		return CMD_ERR;
-- 
2.53.0