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

[PATCH bpf-next v5 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); do_show_subset() has the same double close.
 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 v5:
  - Patch 1 also fixes the double close in do_show_subset() and
    propagates the error when bpf_map_get_info_by_fd() fails on a
    subsequent map in do_dump(), as pointed out in review.
  - Patch 3 detects the truncated read with memchr() for a newline plus
    feof() instead of strlen(), which is also fooled by an embedded NUL
    byte; a line that fills the buffer exactly is no longer rejected,
    as pointed out in review.

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

-- 
2.54.0

[PATCH bpf-next v5 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().

The same double-close pattern exists in do_show_subset(): both
show_map_close_json() and show_map_close_plain() already close the fd,
so drop the extra close() there as well.

Also propagate the error when bpf_map_get_info_by_fd() fails on a
subsequent map in do_dump(): set err = -1 before breaking out of the
loop, so a later failure is not silently hidden after an earlier
iteration succeeded.

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

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 6b9649294ca1..684a8fb72414 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -659,8 +659,6 @@ static int do_show_subset(int argc, char **argv)
 			show_map_close_json(fds[i], &info);
 		else
 			show_map_close_plain(fds[i], &info);
-
-		close(fds[i]);
 	}
 	if (json_output && nb_fds > 1)
 		jsonw_end_array(json_wtr);	/* root array */
@@ -895,7 +893,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;
@@ -944,6 +941,7 @@ static int do_dump(int argc, char **argv)
 	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));
+			err = -1;
 			break;
 		}
 		err = map_dump(fds[i], &info, wtr, nb_fds > 1);
-- 
2.54.0

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

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

Detect the truncated read before stripping the comment, using memchr()
to look for a newline (instead of strlen(), which is also fooled by an
embedded NUL byte) and feof() to tell an overlong line apart from a
final line without a trailing newline. A line that fills the buffer
exactly (the byte after the read is a newline) is not treated as
truncated, so valid maximal-length lines are no longer rejected. Use the
result for the line-length checks, so overlong lines are rejected
regardless of comments or NUL bytes.

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

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 0bbb2e198450..337fbf1ec596 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -367,11 +367,27 @@ static int do_batch(int argc, char **argv)
 	if (json_output)
 		jsonw_start_array(json_wtr);
 	while (fgets(buf, sizeof(buf), fp)) {
+		bool truncated = !memchr(buf, '\n', sizeof(buf) - 1) && !feof(fp);
+
+		if (truncated) {
+			/*
+			 * fgets() filled the buffer. If the very next byte is
+			 * a newline, the line fits the buffer exactly and is
+			 * not truncated.
+			 */
+			int c = fgetc(fp);
+
+			if (c == '\n')
+				truncated = false;
+			else if (c != EOF)
+				ungetc(c, fp);
+		}
+
 		cp = strchr(buf, '#');
 		if (cp)
 			*cp = '\0';
 
-		if (strlen(buf) == sizeof(buf) - 1) {
+		if (truncated) {
 			line_too_long = true;
 			break;
 		}
@@ -380,6 +396,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 +406,15 @@ static int do_batch(int argc, char **argv)
 				goto err_close;
 			}
 
+			cont_truncated = !memchr(contline, '\n', sizeof(contline) - 1) &&
+					 !feof(fp);
+
 			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 v5 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:
- [Medium] Uninitialized memory read in `memchr` when `fgets` reads a short line without a newline.
- [Medium] Discarded newline character breaks the command continuation logic for maximal-length lines.
--

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

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

This commit fixes a bug in bpftool where batch lines containing a '#'
could bypass the line length check, leading to the remainder of the
line being executed as a separate command. It introduces memchr() and
feof() checks to accurately detect truncated reads.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 0bbb2e1984503..337fbf1ec596a 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -367,11 +367,27 @@ static int do_batch(int argc, char **argv)
>  	if (json_output)
>  		jsonw_start_array(json_wtr);
>  	while (fgets(buf, sizeof(buf), fp)) {
> +		bool truncated = !memchr(buf, '\n', sizeof(buf) - 1) && !feof(fp);

[Severity: Medium]
If a short command that does not end in a newline is read, fgets() will
append a null terminator and leave the rest of the stack buffer uninitialized.

Does this memchr() call result in an uninitialized memory read by
unconditionally scanning the full buffer size past the null terminator?

> +
> +		if (truncated) {
> +			/*
> +			 * fgets() filled the buffer. If the very next byte is
> +			 * a newline, the line fits the buffer exactly and is
> +			 * not truncated.
> +			 */
> +			int c = fgetc(fp);
> +
> +			if (c == '\n')
> +				truncated = false;

[Severity: Medium]
When a command line reaches exactly the maximum buffer size minus one and ends
with a continuation backslash, fgets() fills the buffer and stops before
reading the newline.

If we read the newline character here and discard it without putting it back
in the stream or adding it to the buffer, will it break the continuation logic?

Later in do_batch(), the code checks for the presence of the newline alongside
the backslash:

    while ((cp = strstr(buf, "\\\n")) != NULL) {

Will this strstr() check fail since the newline was permanently consumed?

> +			else if (c != EOF)
> +				ungetc(c, fp);
> +		}

[ ... ]

> @@ -380,6 +396,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 +406,15 @@ static int do_batch(int argc, char **argv)
>  				goto err_close;
>  			}
>  
> +			cont_truncated = !memchr(contline, '\n', sizeof(contline) - 1) &&
> +					 !feof(fp);

[Severity: Medium]
Does this memchr() call introduce the same uninitialized memory read regression
for the continuation line buffer as the one above?

> +
>  			cp = strchr(contline, '#');
>  			if (cp)
>  				*cp = '\0';

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