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

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

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