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

[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 error that processes a phantom packet at EOF, leading to an out-of-bounds read and potential crash.
--

commit 0b409950f92375c0cacaee040b899aa78918dfb5
Author: Hasan Basbunar <basbunarhasan@gmail.com>

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

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.

[ ... ]

> diff --git a/tools/bpf/bpf_dbg.c b/tools/bpf/bpf_dbg.c
> index 00e560a17baf..f21576dc2326 100644
> --- a/tools/bpf/bpf_dbg.c
> +++ b/tools/bpf/bpf_dbg.c
> @@ -923,12 +923,12 @@ static bool pcap_next_pkt(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;
>  
>  	pcap_ptr_va_curr += (sizeof(*hdr) + hdr->caplen);
>  	return true;

Does this change cause pcap_next_pkt() to return true when advancing past the
final packet, leaving pcap_ptr_va_curr pointing exactly at the end of the
mapped file boundary?

If the cursor is placed at the end of the file, could this lead to an
out-of-bounds read in commands like run or step?

Looking at cmd_run(), the loop relies on pcap_next_pkt() returning false to
terminate on the final packet:

tools/bpf/bpf_dbg.c:cmd_run() {
	...
	do {
		struct pcap_pkthdr *hdr = pcap_curr_pkt();
		int ret = bpf_run_all(bpf_image, bpf_prog_len,
				      (uint8_t *) hdr + sizeof(*hdr),
				      hdr->caplen, hdr->len);
		...
	} while (pcap_next_pkt() && (!has_limit || (++i < pkts)));
	...
}

If pcap_next_pkt() now returns true when advancing past the final packet, it
appears the loop will execute an extra iteration where pcap_curr_pkt() points
past the valid data, leading to an out-of-bounds dereference of hdr->caplen.

> @@ -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++)
>  		/* noop */;
>  	if (!have_next || pcap_curr_pkt() == NULL) {
>  		rl_printf("no packet #%u available!\n", which);

Does the combination of these changes also cause the select command to
incorrectly succeed when requesting a packet beyond the end of the file?

If have_next receives true when advancing past the final packet, a command
like "select N+1" on an N-packet file will terminate the loop without an error
but leave the cursor positioned at the end of the mapped file boundary,
meaning subsequent commands could read out of bounds.

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