bpf_dbg's interactive 'select ' 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 --- 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@gmail.com/ v2: https://lore.kernel.org/bpf/20260429084441.22089-1-basbunarhasan@gmail.com/ 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