Re: [PATCH v2] bpf: bpf_dbg: fix off-by-one in cmd_select

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces a logic regression in `cmd_select` that skips bounds-checking for the newly selected packet, allowing subsequent commands to blindly dereference invalid or out-of-bounds packets and crash the debugger.
--

commit 34793f5a31a990a20ff41f65cd4c82f4419849d0
Author: Hasan Basbunar <basbunarhasan@gmail.com>

bpf: bpf_dbg: fix off-by-one in cmd_select

This commit changes the loop in cmd_select() to start from 1 instead of 0.
The intent is to fix an off-by-one error where selecting a packet advances
the cursor one packet too far.

> After this fix, 'select K' shows packet K for all K in 1..N, and
> 'select N+1' correctly errors with "no packet #N+1 available!".

Is this description accurate?

For select N+1, the loop will run N times and attempt to validate the
first N packets. If the Nth packet is valid, pcap_next_pkt() returns
true and parks the cursor at the end of the file. Because have_next remains
true, it appears the function will return CMD_OK rather than an error.

Does it only error during testing due to a pre-existing off-by-one check in
pcap_next_pkt() that falsely rejects valid packets aligning exactly with the
file's end? If the PCAP has any trailing garbage bytes, it seems select N+1
would silently succeed.

> diff --git a/tools/bpf/bpf_dbg.c b/tools/bpf/bpf_dbg.c
> index 00e560a17baf..4895602ab37d 100644
> --- a/tools/bpf/bpf_dbg.c
> +++ b/tools/bpf/bpf_dbg.c
> @@ -1141,7 +1141,7 @@ static int cmd_select(char *num)
>  	pcap_reset_pkt();
>  	bpf_reset();
>  
> -	for (i = 0; i < which && (have_next = pcap_next_pkt()); i++)
> +	for (i = 1; i < which && (have_next = pcap_next_pkt()); i++)

Does this change skip bounds checking for the newly selected packet?

Looking at pcap_next_pkt(), it validates the current packet and then
unconditionally advances the cursor. By reducing the loop to which - 1
iterations, it seems cmd_select() positions the cursor on the target packet
but never passes that packet to pcap_next_pkt() for validation.

For example, if which is 1, the loop executes 0 times and have_next remains
true. Could this cause cmd_select() to return CMD_OK even if the PCAP is
empty or the first packet is truncated?

If so, could subsequent commands like run or step blindly dereference the
unvalidated packet from pcap_curr_pkt(), potentially causing an
out-of-bounds memory read?

>  		/* noop */;
>  	if (!have_next || pcap_curr_pkt() == NULL) {
>  		rl_printf("no packet #%u available!\n", which);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429084441.22089-1-basbunarhasan@gmail.com?part=1