Re: [PATCH bpf-next v6 6/9] selftests/bpf: make cmdline_contains stricter

[bot+bpf-ci@kernel.org]

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

> selftests/bpf: make cmdline_contains stricter
>
> cmdline_contains is used by BPF selftests to check the presence of
> specific kernel commandline parameters, but it currently suffers from
> two issues:
> - the read commandline isn't NULL terminated right after the read data
>   but only at the end of the buffer, leaving uninitialized bytes that
>   are then possibly tokenized
> - the comparison of found tokens is done based on the size of found
>   token. This could lead to too-short-but-matching tokens to wrongly
>   match the search pattern.
>
> Enforce stricter checks in cmdline_contains to avoid accidental matches.
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

The commit message describes two concrete bugs in existing selftest
code ('the read commandline isn't NULL terminated right after the read
data but only at the end of the buffer, leaving uninitialized bytes
that are then possibly tokenized' and 'the comparison of found tokens
is done based on the size of found token. This could lead to
too-short-but-matching tokens to wrongly match the search pattern'),
but there is no Fixes: tag.

Both defects were introduced together by 399f6185a1c0 ("selftests/bpf:
Fix selftests broken by mitigations=off"), which added the original
helper:

    +	if (read(fd, cmdline, sizeof(cmdline) - 1) < 0) {
    ...
    +	cmdline[sizeof(cmdline) - 1] = '\0';
    +	for (c = strtok(cmdline, " \n"); c; c = strtok(NULL, " \n")) {
    +		if (strncmp(c, "mitigations=off", strlen(c)))

The late NUL termination and the strncmp(c, pat, strlen(c)) prefix
comparison are both present verbatim in that commit. The
prefix-compare bug is real and reachable: a one-character token such
as 'm' on the command line yields strlen(c) == 1, so strncmp(c,
"mitigations=off", 1) returns 0 and the helper reports a match,
causing get_unpriv_disabled() to skip the unprivileged verifier tests
on a machine where mitigations are actually enabled.

Should this include:

  Fixes: 399f6185a1c0 ("selftests/bpf: Fix selftests broken by mitigations=off")

> diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
> index f997d7ec8fd08..c99d81df2aa25 100644
> --- a/tools/testing/selftests/bpf/unpriv_helpers.c
> +++ b/tools/testing/selftests/bpf/unpriv_helpers.c
> @@ -72,8 +72,9 @@ static int config_contains(const char *pat)
>
>  static bool cmdline_contains(const char *pat)
>  {
> +	int fd, cnt, ret = false;
>  	char cmdline[4096], *c;
> -	int fd, ret = false;
> +	size_t pat_len;
>
>  	fd = open("/proc/cmdline", O_RDONLY);
>  	if (fd < 0) {
> @@ -81,14 +82,16 @@ static bool cmdline_contains(const char *pat)
>  		return false;
>  	}
>
> -	if (read(fd, cmdline, sizeof(cmdline) - 1) < 0) {
> +	cnt = read(fd, cmdline, sizeof(cmdline) - 1);
> +	if (cnt < 0) {
>  		perror("read /proc/cmdline");
>  		goto out;
>  	}
>
> -	cmdline[sizeof(cmdline) - 1] = '\0';
> +	cmdline[cnt] = '\0';
> +	pat_len = strlen(pat);
>  	for (c = strtok(cmdline, " \n"); c; c = strtok(NULL, " \n")) {
> -		if (strncmp(c, pat, strlen(c)))
> +		if (strlen(c) != pat_len || strcmp(c, pat))
>  			continue;
>  		ret = true;
>  		break;

This isn't a bug, but since strcmp() already fails on strings of
different lengths, would `if (strcmp(c, pat))` on its own be enough
here, letting `pat_len` go away and matching how config_contains()
does the same check?

The sibling helper in the same file expresses the same idea with a
bare strcmp:

    config_contains(), unpriv_helpers.c:
        if (strcmp(buf, pat) == 0) {


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