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

[PATCH bpf-next v3 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. (new in v3) 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.

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

Changes in v2:
  - Patch 2 now uses ferror() to detect read failures and tracks an
    overlong line explicitly instead of relying on errno; Fixes tag
    corrected to 71bb428fe2c1.

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 | 12 +++++++++---
 tools/bpf/bpftool/map.c  |  1 -
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.54.0

[PATCH bpf-next v3 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 v3 1/3] bpftool: fix double close in map dump

[bot+bpf-ci]

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

> 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;

This isn't a bug, but since exit_free is map_dump()'s common exit and
the close ran on the success path too (and the break path also
double-closed via exit_close), would it read more accurately as something
like "map_dump() closes the map fd on all exit paths, and do_dump() closes
it again"?


---
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/31367518511

[PATCH bpf-next v3 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.

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

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index c91e1a6e1a1e..c9a86039c400 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 {
-- 
2.54.0

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

[bot+bpf-ci]

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

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index c91e1a6e1a1e..c9a86039c400 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c

[ ... ]

> @@ -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 {

[ ... ]

This isn't a bug, but now that the batch read check uses ferror() rather
than errno, is the USE_LIBCAP errno = 0 reset in main() still needed?

Looking at main():

#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

The comment says the reset is specifically to prevent interference with
batch mode, but do_batch() no longer checks errno to detect read failures.
Could this block be dropped alongside the ferror() change?


---
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/31367518511

[PATCH bpf-next v3 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.

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

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

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index c9a86039c400..c77e66bffbc9 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;
 		}
-- 
2.54.0

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

[sashiko-bot]

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

Pre-existing issues:
- [Medium] The patch fixes the batch line length check bypass in the main buffer but misses the identical vulnerability in continuation lines, allowing command smuggling.
- [Medium] The newly introduced `truncated` check relies on `strlen()`, which can be bypassed by injecting a null byte, re-enabling the command smuggling vulnerability.
--

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

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

This commit attempts to fix a command smuggling vulnerability where overlong
lines containing a '#' character bypass the buffer length check. It records
whether fgets() truncated the line before stripping the comment to ensure
overlong lines are properly rejected.

> 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: Medium]
This isn't a bug introduced by this patch, but rather a pre-existing issue.
Can the new truncated check be bypassed if the input contains a null byte?

Since fgets() reads up to sizeof(buf) - 1 bytes without stopping at null
bytes, but strlen() stops at the first null byte, wouldn't an overlong line
with an injected null byte result in a smaller strlen(buf)?

If that happens, wouldn't the truncated check evaluate to false, bypassing the
line limit and leaving the remainder of the overlong line in the file stream
to be parsed as a separate command?

> +
>  		cp = strchr(buf, '#');
>  		if (cp)
>  			*cp = '\0';
>  
> -		if (strlen(buf) == sizeof(buf) - 1) {
> +		if (truncated) {
>  			line_too_long = true;
>  			break;
>  		}

[Severity: Medium]
This is a pre-existing issue and was not introduced by the patch under review.
Does the same command smuggling vulnerability still exist for continuation
lines?

Looking further down in do_batch(), continuation lines are handled similarly:

tools/bpf/bpftool/main.c:do_batch() {
    ...
    while ((cp = strstr(buf, "\\\n")) != NULL) {
        if (!fgets(contline, sizeof(contline), fp) || ...)
            ...
        cp = strchr(contline, '#');
        if (cp)
            *cp = '\0';

        if (strlen(buf) + strlen(contline) + 1 > sizeof(buf)) {
            p_err("command %u is too long", lines);
    ...
}

If a continuation line exceeds the buffer size and contains a '#' character
within the first 4095 bytes, it will be prematurely truncated before the
length check.

Because this artificially reduces strlen(contline), doesn't this bypass the
bounds check and leave the unread remainder of the overlong continuation line
in the file stream to be executed as an unintended command?

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

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

[bot+bpf-ci]

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

>     Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
>     Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>

Does the Fixes: tag point to the right commit?

The bug being fixed is the ordering of the comment strip relative to the
line-length check. In commit 71bb428fe2c1 ("tools: bpf: add bpftool")
do_batch() had no '#' handling at all; the loop body started with the
length check immediately after fgets():

    while (fgets(buf, sizeof(buf), fp)) {
            if (strlen(buf) == sizeof(buf) - 1) {
                    errno = E2BIG;
                    break;
            }

So an overlong line was correctly rejected with E2BIG and the bypass
described in the changelog was not possible.

The defect was introduced by commit 06cc7fe7c295 ("tools: bpftool: support
comments in batch files"), which inserted the truncation directly above
that check:

    while (fgets(buf, sizeof(buf), fp)) {
    +       cp = strchr(buf, '#');
    +       if (cp)
    +               *cp = '\0';
    +
            if (strlen(buf) == sizeof(buf) - 1) {

That is the commit that made strlen(buf) shrink before the length test,
which is exactly what this patch reorders. The tag should read:

    Fixes: 06cc7fe7c295 ("tools: bpftool: support comments in batch files")

This also matters for stable backport scope: 71bb428fe2c1 landed in v4.15
while 06cc7fe7c295 landed in v4.16, so the current tag widens the backport
range to a kernel where the bug is absent.

Note the parent commit fbbb15fafea3 ("bpftool: fix spurious batch file read
error") does correctly carry Fixes: 71bb428fe2c1, since the errno-based
check it replaces was present in that commit; the tag appears to have been
carried over unchanged into this patch.

The code change itself is correct: recording 'truncated' from fgets()
before the '#' strip makes the E2BIG rejection independent of comments,
and the new local is block-scoped inside the loop body so it is
re-initialized on every iteration.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index c9a86039c400..c77e66bffbc9 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;
>  		}


---
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/31367518511