[PATCH bpf-next v4 0/3] bpftool: fix batch file handling issues

[PATCH bpf-next v4 0/3] bpftool: fix batch file handling issues

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

This series fixes three issues in bpftool's batch file handling:

 1. map dump closes the map fd twice (introduced by the map-by-name
    dump refactor).
 2. do_batch() reports a spurious read failure because a stale errno
    from a previously executed command is used instead of checking the
    FILE stream state.
 3. a line longer than the batch buffer that contains a '#' within the
    buffered prefix bypasses the line length check: the comment
    stripping truncates the buffer before the length is checked, so the
    unread remainder of the line is parsed and executed as a separate
    command on the next loop iteration. Continuation lines are affected
    the same way, bypassing the "command is too long" check.

Changes in v4:
  - Extend patch 3 to also cover continuation lines, whose comment
    stripping bypasses the "command is too long" check the same way, as
    pointed out in review.
  - Drop the USE_LIBCAP errno reset in main(), which existed only to
    keep errno clean for the batch mode and is no longer needed now
    that do_batch() detects read failures with ferror(), as pointed out
    in review.

Changes in v3:
  - Add patch 3 fixing the pre-existing comment-stripping bypass of the
    batch line length check, as pointed out in review.

Yuan Chen (3):
  bpftool: fix double close in map dump
  bpftool: fix spurious batch file read error
  bpftool: Fix bypass of the batch line length check by comments

 tools/bpf/bpftool/main.c | 29 +++++++++++++++--------------
 tools/bpf/bpftool/map.c  |  1 -
 2 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.54.0

[PATCH bpf-next v4 1/3] bpftool: fix double close in map dump

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

map_dump() closes the map fd in its error path, and do_dump() then
closes the same fd again after a successful dump. Closing an already
closed fd leaves errno set to EBADF, which poisons later errno checks
such as the batch file read check in do_batch(). Let do_dump() own the
fd and remove the close from map_dump().

Fixes: 99f9863a0c45f ("bpftool: Match maps by name")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 tools/bpf/bpftool/map.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 6b9649294ca1..e9f4dd9458cf 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -895,7 +895,6 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
 exit_free:
 	free(key);
 	free(value);
-	close(fd);
 	free_map_kv_btf(btf);
 
 	return err;
-- 
2.54.0

Re: [PATCH bpf-next v4 1/3] bpftool: fix double close in map dump

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Missing error propagation when `bpf_map_get_info_by_fd()` fails on subsequent maps in `do_dump()`.
- [Medium] Double close of map file descriptors in `do_show_subset()`.
--

--- Patch [1]: [PATCH bpf-next v4 1/3] bpftool: fix double close in map dump ---
commit 8a5aea6bcad40d4506c71d6f8f2b151ddbe0ed20
Author: Yuan Chen <chenyuan@kylinos.cn>

bpftool: fix double close in map dump

This commit fixes a double close in map_dump() where the map file
descriptor was being closed in the error path, and then closed again
afterward by do_dump().

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 6b9649294ca1a..e9f4dd9458cf1 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -895,7 +895,6 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>  exit_free:
>  	free(key);
>  	free(value);
> -	close(fd);
>  	free_map_kv_btf(btf);
>  
>  	return err;

[Severity: Medium]
This is a pre-existing issue, but I noticed another double close pattern 
in tools/bpf/bpftool/map.c:do_show_subset():

tools/bpf/bpftool/map.c:do_show_subset() {
    ...
		if (json_output)
			show_map_close_json(fds[i], &info);
		else
			show_map_close_plain(fds[i], &info);

		close(fds[i]);
    ...
}

Since the show_map_close_json and show_map_close_plain functions already
unconditionally close the file descriptor, does the subsequent close(fds[i])
result in a double close?

This seems like it would also poison later errno checks exactly like the
bug this patch addresses.


[Severity: Medium]
This is a pre-existing issue, but while looking at do_dump() in the same 
file, there appears to be a missing error propagation:

tools/bpf/bpftool/map.c:do_dump() {
    ...
	for (i = 0; i < nb_fds; i++) {
		if (bpf_map_get_info_by_fd(fds[i], &info, &len)) {
			p_err("can't get map info: %s", strerror(errno));
			break;
		}
		err = map_dump(fds[i], &info, wtr, nb_fds > 1);
    ...
}

If a user executes bpftool with multiple map file descriptors, and
bpf_map_get_info_by_fd() fails on any iteration after the first, the loop
breaks without updating err.

Doesn't this incorrectly cause bpftool to exit with a success status if
the previous iteration succeeded, silently hiding the error from scripts or
users relying on the exit code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810090459.2666243-1-chenyuan_fl@163.com?part=1

[PATCH bpf-next v4 2/3] bpftool: fix spurious batch file read error

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

do_batch() checks errno after the read loop to detect read failures,
but fgets() does not clear errno on success, so a stale errno left by
a previously executed command (e.g. map dump's EBADF from a double
close) makes bpftool report a batch file read failure and exit with an
error even though every command succeeded. Use ferror() instead, and
track the too-long-line case explicitly.

Since do_batch() no longer inspects errno to detect read failures, drop
the USE_LIBCAP errno reset in main() that existed only to keep errno
clean for the batch mode.

Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 tools/bpf/bpftool/main.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index c91e1a6e1a1e..0bbb2e198450 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -336,6 +336,7 @@ static int do_batch(int argc, char **argv)
 	char buf[BATCH_LINE_LEN_MAX], contline[BATCH_LINE_LEN_MAX];
 	char *n_argv[BATCH_ARG_NB_MAX];
 	unsigned int lines = 0;
+	bool line_too_long = false;
 	int n_argc;
 	FILE *fp;
 	char *cp;
@@ -371,7 +372,7 @@ static int do_batch(int argc, char **argv)
 			*cp = '\0';
 
 		if (strlen(buf) == sizeof(buf) - 1) {
-			errno = E2BIG;
+			line_too_long = true;
 			break;
 		}
 
@@ -429,7 +430,10 @@ static int do_batch(int argc, char **argv)
 		lines++;
 	}
 
-	if (errno && errno != ENOENT) {
+	if (line_too_long) {
+		p_err("reading batch file failed: %s", strerror(E2BIG));
+		err = -1;
+	} else if (ferror(fp)) {
 		p_err("reading batch file failed: %s", strerror(errno));
 		err = -1;
 	} else {
@@ -467,16 +471,6 @@ int main(int argc, char **argv)
 
 	setlinebuf(stdout);
 
-#ifdef USE_LIBCAP
-	/* Libcap < 2.63 hooks before main() to compute the number of
-	 * capabilities of the running kernel, and doing so it calls prctl()
-	 * which may fail and set errno to non-zero.
-	 * Let's reset errno to make sure this does not interfere with the
-	 * batch mode.
-	 */
-	errno = 0;
-#endif
-
 	last_do_help = do_help;
 	pretty_output = false;
 	json_output = false;
-- 
2.54.0

Re: [PATCH bpf-next v4 2/3] bpftool: fix spurious batch file read error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The batch file line length checking logic in `do_batch` is fundamentally flawed, leading to three issues: 1) It incorrectly rejects valid lines that are exactly 4095 bytes (including the newline). 2) It allows oversized lines with comments to bypass the length limit because the truncation check is performed after comment stripping. 3) It fails to validate continuation lines properly. These parser flaws can cause the unread remainder of an oversized line to be executed as a completely new, hidden command.
--

--- Patch [2]: [PATCH bpf-next v4 2/3] bpftool: fix spurious batch file read error ---
commit cf1f390638590ec88aac09a2ed99cf49c783fe7f
Author: Yuan Chen <chenyuan@kylinos.cn>

bpftool: fix spurious batch file read error

This commit fixes a spurious batch file read error in bpftool. It replaces
the use of errno with ferror() to detect read failures and explicitly tracks
line length truncations.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index c91e1a6e1a1ec..0bbb2e1984503 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
[ ... ]
> @@ -371,7 +372,7 @@ static int do_batch(int argc, char **argv)
>  			*cp = '\0';
>  
>  		if (strlen(buf) == sizeof(buf) - 1) {
> -			errno = E2BIG;
> +			line_too_long = true;

[Severity: High]
This is a pre-existing issue, but by checking the string length after
stripping comments, couldn't an oversized line containing a '#' bypass this
truncation check?

If a long line is read and contains a '#', strchr() will null-terminate
it early, making strlen(buf) smaller than sizeof(buf) - 1. The unread
remainder of that oversized line would then be left in the file stream
and executed as a new command on the next fgets() iteration.

Additionally, does this logic incorrectly reject valid lines that fit
the buffer exactly?

This same parser flaw appears to exist for continuation lines where
lengths are also checked after comment stripping.

Should the truncation check be performed immediately after fgets(), before
any characters are modified?

>  			break;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810090459.2666243-1-chenyuan_fl@163.com?part=2

Re: [PATCH bpf-next v4 2/3] bpftool: fix spurious batch file read error

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

> bpftool: fix spurious batch file read error

This isn't a bug, but would 'bpftool: Fix spurious batch file read error'
match the capitalisation used by the other bpftool patches in this series?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31373774115

[PATCH bpf-next v4 3/3] bpftool: Fix bypass of the batch line length check by comments

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

do_batch() strips trailing comments by truncating the line at '#'
before checking whether fgets() filled the buffer. If a batch line
longer than the buffer contains a '#' within the first
sizeof(buf) - 1 bytes, the truncation makes strlen(buf) smaller and the
line-length check is bypassed. The unread remainder of the line then
stays in the file stream and is parsed and executed as a separate
command on the next loop iteration.

Continuation lines handled below are affected the same way: an overlong
continuation line containing '#' bypasses the "command is too
long" check, and its unread remainder is executed as a separate command.

Record whether fgets() truncated the line before stripping the comment,
and use that for the line-length checks, so overlong lines are rejected
regardless of comments.

Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 tools/bpf/bpftool/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 0bbb2e198450..cef779aa72fb 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -367,11 +367,13 @@ static int do_batch(int argc, char **argv)
 	if (json_output)
 		jsonw_start_array(json_wtr);
 	while (fgets(buf, sizeof(buf), fp)) {
+		bool truncated = strlen(buf) == sizeof(buf) - 1;
+
 		cp = strchr(buf, '#');
 		if (cp)
 			*cp = '\0';
 
-		if (strlen(buf) == sizeof(buf) - 1) {
+		if (truncated) {
 			line_too_long = true;
 			break;
 		}
@@ -380,6 +382,8 @@ static int do_batch(int argc, char **argv)
 		 * with '\' in the batch file).
 		 */
 		while ((cp = strstr(buf, "\\\n")) != NULL) {
+			bool cont_truncated;
+
 			if (!fgets(contline, sizeof(contline), fp) ||
 			    strlen(contline) == 0) {
 				p_err("missing continuation line on command %u",
@@ -388,11 +392,14 @@ static int do_batch(int argc, char **argv)
 				goto err_close;
 			}
 
+			cont_truncated = strlen(contline) == sizeof(contline) - 1;
+
 			cp = strchr(contline, '#');
 			if (cp)
 				*cp = '\0';
 
-			if (strlen(buf) + strlen(contline) + 1 > sizeof(buf)) {
+			if (cont_truncated ||
+			    strlen(buf) + strlen(contline) + 1 > sizeof(buf)) {
 				p_err("command %u is too long", lines);
 				err = -1;
 				goto err_close;
-- 
2.54.0

Re: [PATCH bpf-next v4 3/3] bpftool: Fix bypass of the batch line length check by comments

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for batch line length check bypass, allowing command injection via embedded null bytes.
--

--- Patch [3]: [PATCH bpf-next v4 3/3] bpftool: Fix bypass of the batch line length check by comments ---
commit 1803699d659bb5e5e3beb9beeb6577cdf2791dda
Author: Yuan Chen <chenyuan@kylinos.cn>

bpftool: Fix bypass of the batch line length check by comments

This commit fixes an issue in do_batch() where trailing comments truncated
by '#' would bypass the line length check. It records whether fgets()
truncated the line before stripping the comment to reliably reject overlong
lines regardless of comments.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -367,11 +367,13 @@ static int do_batch(int argc, char **argv)
>  	if (json_output)
>  		jsonw_start_array(json_wtr);
>  	while (fgets(buf, sizeof(buf), fp)) {
> +		bool truncated = strlen(buf) == sizeof(buf) - 1;

[Severity: High]
This isn't a bug introduced by this patch, but does using strlen() here leave
the length check vulnerable to an embedded null byte bypass?

Since fgets() reads up to sizeof(buf) - 1 bytes, if a batch file line
contains an embedded null byte ('\0'), strlen(buf) will stop at the null
byte. This makes strlen(buf) strictly less than sizeof(buf) - 1, causing the
truncation check to evaluate to false even if the line exceeded the buffer
capacity.

If this bypasses the length check, could the unread remainder of the
buffer-exceeding line remain in the file stream and be parsed and executed
as a separate, arbitrary command on the next loop iteration? 

Might it be safer to check if the buffer is missing a newline character
instead of relying on strlen()?

[ ... ]

>  		/* Append continuation lines if any (coming after a line ending
>  		 * with '\' in the batch file).
>  		 */
>  		while ((cp = strstr(buf, "\\\n")) != NULL) {
> +			bool cont_truncated;
> +
>  			if (!fgets(contline, sizeof(contline), fp) ||
>  			    strlen(contline) == 0) {
[ ... ]
>  			}
>  
> +			cont_truncated = strlen(contline) == sizeof(contline) - 1;

[Severity: High]
This is also a pre-existing issue, but does the same embedded null byte
bypass apply to continuation lines here?

If contline contains a null byte, cont_truncated will evaluate to false,
potentially allowing an overlong continuation line to bypass the length
enforcement and execute trailing input as unintended commands.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810090459.2666243-1-chenyuan_fl@163.com?part=3