Skip to content

Commit 2494c4d

Browse files
hasso5703Kernel Patches Daemon
authored andcommitted
bpf: bpf_dbg: split pcap_next_pkt() validation/advance, fix off-by-one in cmd_select
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: fd981e3 ("filter: bpf_dbg: add minimal bpf debugger") Signed-off-by: Hasan Basbunar <basbunarhasan@gmail.com>
1 parent 6d81208 commit 2494c4d

1 file changed

Lines changed: 14 additions & 5 deletions

File tree

tools/bpf/bpf_dbg.c

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -918,21 +918,30 @@ static struct pcap_pkthdr *pcap_curr_pkt(void)
918918
return (void *) pcap_ptr_va_curr;
919919
}
920920

921-
static bool pcap_next_pkt(void)
921+
static bool pcap_curr_pkt_valid(void)
922922
{
923923
struct pcap_pkthdr *hdr = pcap_curr_pkt();
924924

925925
if (pcap_ptr_va_curr + sizeof(*hdr) -
926-
pcap_ptr_va_start >= pcap_map_size)
926+
pcap_ptr_va_start > pcap_map_size)
927927
return false;
928928
if (hdr->caplen == 0 || hdr->len == 0 || hdr->caplen > hdr->len)
929929
return false;
930930
if (pcap_ptr_va_curr + sizeof(*hdr) + hdr->caplen -
931-
pcap_ptr_va_start >= pcap_map_size)
931+
pcap_ptr_va_start > pcap_map_size)
932932
return false;
933+
return true;
934+
}
935+
936+
static bool pcap_next_pkt(void)
937+
{
938+
struct pcap_pkthdr *hdr;
933939

940+
if (!pcap_curr_pkt_valid())
941+
return false;
942+
hdr = pcap_curr_pkt();
934943
pcap_ptr_va_curr += (sizeof(*hdr) + hdr->caplen);
935-
return true;
944+
return pcap_curr_pkt_valid();
936945
}
937946

938947
static void pcap_reset_pkt(void)
@@ -1143,7 +1152,7 @@ static int cmd_select(char *num)
11431152

11441153
for (i = 0; i < which && (have_next = pcap_next_pkt()); i++)
11451154
/* noop */;
1146-
if (!have_next || pcap_curr_pkt() == NULL) {
1155+
if (!have_next || !pcap_curr_pkt_valid()) {
11471156
rl_printf("no packet #%u available!\n", which);
11481157
pcap_reset_pkt();
11491158
return CMD_ERR;

0 commit comments

Comments
 (0)