From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CA02729BD95 for ; Wed, 29 Apr 2026 12:35:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777466153; cv=none; b=M9F6mh5RD913sznDsN2GxmYPNsLn5GBqYlWZVtekARDeJFEiE0yuSXjcwFs78sxLN3g3ebm7dz+c5ZUOTgA7fGqrRQqAO9b2iZ9xGFMYJXOy6yE+l0KCKVgenQtXaNrnpc1+u7gPuVCMdKKcWYk272TiAwhxbp6BFVh7ilHVb9Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777466153; c=relaxed/simple; bh=vqut0vK5KntPrajlpkM2HSqc9+hZY4ZQ0CEjyeDWeyE=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=VgA9USK5/tlMCyRqYf9ItUD/DPia4mP3Jx8CIDdvsOudwkxn8SQgUI/5nS1EDM2LKvgNnTcrYUzID01IIrwQrtuWor7EO9XsqD+cYkXRU4m3ASYoOjm1cYh/pUqNRbRFceiC28SlJyG2eIzTwBeD5IJK2XvBzw9YmonkS75Ge3Q= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=EzkVfNGW; arc=none smtp.client-ip=209.85.128.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="EzkVfNGW" Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-488ab2db91aso176663325e9.3 for ; Wed, 29 Apr 2026 05:35:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1777466148; x=1778070948; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=jZAFdTvgv7ZDBz5Q2Pc7VeLQwtxOy5o6AziVPA6ZPDE=; b=EzkVfNGWmv1lQp7qcqD1g2U85iBvrLpaZu5+Ic3JuufXrCd8ePU9Bzt2BI8K+YyY/q oW3CJP+VUD9kqquu5w7mYw7MHghCYy+ipYvzziCq0nfnBKYMKAYNM17BtxMJAsDvZXAk e/NuYfpToQXyI2zuqaU6TvFQCBJuSNahEQ7GI0yJF0Xhc99Rcz5VGp7Bt1H6/oMWIya2 k0uuJR//duKK0uRwxWS2QNVXehkE5ab3u0KoUKqrkdRssClGwv3sD1f5xb99GSxnTXTw 0Ns747bQ/VzzK7nmbKyG1d4aOQFK36RP2U3YKKUHt5WZmSeqtmo05AyzUAC/3iXV2Nry cwYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777466148; x=1778070948; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=jZAFdTvgv7ZDBz5Q2Pc7VeLQwtxOy5o6AziVPA6ZPDE=; b=aSFffUSTbL9H/GxAMfQEtsRl85dGmP3AaNxgA5vRD2TfYCLYFrtD3k3ky+0RcJtFTd VdGjS2tc2KSAFwEs5XmZEBd7kmhBAUzwblGQpVlz4hr9mvaTC5dSzGLmT9dDXZ8VcpAp U5uFKGeMSWp462POqzBoP+HgkhnuCkn+K74/aIIwri9Wjj725IH9I+Ayir9ANXG+zOVZ 8lUwqjjWm3EGtAE427Xcrm0d5TLVbgBRs45kBINcGfk3hvMkCvpxZhQY0UA/Qol/+VUA SqCtYqlvjXhr0/VR2mtou6n99fwSF3n9cf/Pez6RpdOeoPuaQ6VxtXin37c0JR42C43A /Phw== X-Forwarded-Encrypted: i=1; AFNElJ8fD5/EOBVzsbEJ9EskmrTzdWwcVcZMs3J7Bd5RxlHWYuUx939bpLNTL8rSE5wdQNUiBXZM/5+TJ7x2WPQ=@vger.kernel.org X-Gm-Message-State: AOJu0Yw0HjgdQZ49d8DJx+ca7ye3dSrtnnvJ+/yLlGSiXO3b9R0uTxGQ xIXVXef4Bq/D6sEyjNifLBHz3SsqkvosCjyKnLQHdLsAWFHRdbmLuMYrDJ8G3SSr X-Gm-Gg: AeBDiesw6+E0FiZz2s0o0L2r5B39C8h+Rr5SYLOuoMbdOHCsI+am8SA49dvJvImpz0q 2OpVHA2Zs/tuGGBUjHzO2vOFsGRjq+Ps/JTTZ5yfGcJPXjX/04Yet2m9bsCWoyQUVOm6c8IU4s2 BjqPh/C+WxI4rjM+5V35u4Knx0cqqis2/r1Dgp6yUwwUPZuaNP6Mxkb7Uka2PtedgpvfVpqKlxs HN9xWUSb7T094t/OvfHzx950RapNTIf7ZUQ2lZ/2GFFm2EkrMZ0FbHx3XsT6Zr+99u6S09nenvK 3XFx/MOyyT0hw5hx6MxoGyVi2XChecdBn9PyudzYKbwQsRANExZRZXZtjB/slYgEkuXn/oPBQNH yF+unIgfIqo6wX4LN7CDR5476krCd89+6GPOy/Nmn/ZB0qgkQEreO4xvY816pw3/HgeWBYEvasW o8K3uqDb48RJesss6acK5hyGxPTvjZfD0BsGGpCXd6FjaoppOxYLy1jzYvXlsiIiyO8YtWmJvHB 4gOvw== X-Received: by 2002:a05:600c:314d:b0:48a:58ae:992f with SMTP id 5b1f17b1804b1-48a7b531975mr56915905e9.16.1777466147586; Wed, 29 Apr 2026 05:35:47 -0700 (PDT) Received: from localhost.localdomain ([2a05:6e02:1135:da10:ada7:48a3:13c3:5e89]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48a7bc12bcbsm86942355e9.1.2026.04.29.05.35.46 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Wed, 29 Apr 2026 05:35:47 -0700 (PDT) From: Hasan Basbunar To: Daniel Borkmann Cc: Alexei Starovoitov , Andrii Nakryiko , bpf@vger.kernel.org, linux-kernel@vger.kernel.org, Hasan Basbunar Subject: [PATCH v3] bpf: bpf_dbg: split pcap_next_pkt() validation/advance, fix off-by-one in cmd_select Date: Wed, 29 Apr 2026 14:35:43 +0200 Message-ID: <20260429123543.61559-1-basbunarhasan@gmail.com> X-Mailer: git-send-email 2.53.0 In-Reply-To: <20260429084441.22089-1-basbunarhasan@gmail.com> References: <20260429084441.22089-1-basbunarhasan@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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