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

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 <[email protected]>

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/[email protected]?part=3