Re: [PATCH bpf-next v4 2/2] selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] `__bpf_strncat` contains a critical out-of-bounds read and potential kernel panic when `dst` is not null-terminated.
- [High] The new failure tests passing scalar or invalid pointers to `bpf_strcat` and `bpf_strncat` will be rejected by the verifier, breaking the selftest suite.
- [High] The `test_strncat_success` test asserts an incorrect return value and checks against the wrong string, guaranteeing runtime failure.
- [High] The `src` argument in `bpf_strcat` and `bpf_strncat` lacks a string or size suffix, causing the verifier to only enforce a 1-byte size, leading to kernel memory disclosure.
- [High] `run_too_long_tests` asserts `-E2BIG` but `__bpf_strncat` returns `-EINVAL`, breaking the test suite.
--

--- Patch [2]: [PATCH bpf-next v4 2/2] selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs ---
commit 08f044c5d96948cad08013caa519f6e718db089c
Author: Rong Tao <rongtao@cestc.cn>

selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs

This patch adds selftests for the newly introduced bpf_strcat() and
bpf_strncat() kfuncs. It extends existing string kfunc failure and success
test suites to validate these operations. It also cleans up some extra
whitespace in string_kfuncs_failure1.c.

> diff --git a/tools/testing/selftests/bpf/prog_tests/string_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/string_kfuncs.c
> index 300032a194451..460567ef622a5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/string_kfuncs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/string_kfuncs.c
> @@ -24,6 +24,8 @@ static const char * const test_cases[] = {
>  	"strcasestr",
>  	"strnstr",
>  	"strncasestr",
> +	"strcat",
> +	"strncat",
>  };

[Severity: High]
Will run_too_long_tests() fail for these new test cases due to a mismatched
error code?

The run_too_long_tests() loop expects all over-long string tests to return
-E2BIG. However, looking at __bpf_strncat() in kernel/bpf/helpers.c, if the
destination buffer is full because it lacks a null terminator within dsz
bounds:

    if (dlen >= dsz || sz == 0 || dsz == 0)
        return -EINVAL;

Since the test string is completely filled without a null terminator, wouldn't
it return -EINVAL instead of -E2BIG, causing the ASSERT_EQ to fail?

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> index bddc4e8579d24..d432d4990f802 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
[ ... ]
> @@ -30,29 +31,31 @@ char *invalid_kern_ptr = (char *)-1;
>  
>  /* Passing NULL to string kfuncs (treated as a userspace ptr) */
[ ... ]
> +SEC("syscall") __retval(USER_PTR_ERR)int test_strcat_null1(void *ctx) { return bpf_strcat(kern_buf, sizeof(kern_buf), NULL); }
> +SEC("syscall") __retval(USER_PTR_ERR)int test_strncat_null1(void *ctx) { return bpf_strncat(kern_buf, sizeof(kern_buf), NULL, 2); }
>  
>  /* Passing userspace ptr to string kfuncs */
[ ... ]
> +SEC("syscall") __retval(USER_PTR_ERR) int test_strcat_user_ptr1(void *ctx) { return bpf_strcat(user_ptr, 1, kern_buf); }

[Severity: High]
Will these tests fail to load during verification rather than failing at
runtime?

Because the dst and src parameters in bpf_strcat() lack the __ign or
__nullable suffix in their kernel definitions, the verifier treats them as
strictly typed KF_ARG_PTR_TO_MEM.

If we pass a scalar like user_ptr or NULL, won't the verifier reject the
program during the load phase, breaking the test suite when the skeleton is
loaded?

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
> index 412c53b87b182..2dbd1349689ea 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
[ ... ]
> @@ -23,5 +24,7 @@ SEC("syscall") int test_strstr_too_long(void *ctx) { return bpf_strstr(long_str,
>  SEC("syscall") int test_strcasestr_too_long(void *ctx) { return bpf_strcasestr(long_str, "hello"); }
>  SEC("syscall") int test_strnstr_too_long(void *ctx) { return bpf_strnstr(long_str, "hello", sizeof(long_str)); }
>  SEC("syscall") int test_strncasestr_too_long(void *ctx) { return bpf_strncasestr(long_str, "hello", sizeof(long_str)); }
> +SEC("syscall") int test_strcat_too_long(void *ctx) { return bpf_strcat(long_str, sizeof(long_str), str); }

[Severity: Critical]
Could this test trigger an out-of-bounds read and kernel panic?

Looking at __bpf_strncat() in kernel/bpf/helpers.c, when calculating dlen
there is no bounds check against dsz:

    while (dst[dlen] != '\0')
        dlen++;

If test_strcat_too_long() passes a completely filled long_str without a null
terminator, won't this loop read past the end of the BPF-provided buffer into
adjacent kernel memory?

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> index f65b1226a81a3..71fbbe97b4c50 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
[ ... ]
> +__test(0) int test_strcat_success(void *ctx) {
> +	char buf[32] = "hello";
> +	bpf_strcat(buf, sizeof(buf), str);
> +	return bpf_strcmp(buf, "hellohello world");
> +}

[Severity: High]
Does the signature of bpf_strcat() allow arbitrary kernel memory disclosure?

The src parameter in bpf_strcat() is annotated simply as const char *src,
lacking a __str suffix or a paired size argument in kernel/bpf/helpers.c:

    __bpf_kfunc int bpf_strcat(char *dst, u32 dst__sz, const char *src)

This causes the verifier to check only the size of the base type (1 byte).
If a BPF program passes a valid 1-byte memory region, could
strncpy_from_kernel_nofault() safely copy past the 1-byte verified region,
exposing adjacent kernel memory into dst?

> +__test(8) int test_strncat_success(void *ctx) {
> +	char buf[32] = "hello";
> +	bpf_strncat(buf, sizeof(buf), str, 3);
> +	return bpf_strcmp(buf, "hellohe");
> +}

[Severity: High]
Is the expected return value correct here?

The test uses __test(8), expecting the program to return 8. However,
bpf_strcmp() evaluates to 0 on a successful match. Wouldn't the program
return 0 instead of 8, causing this test to fail reliably?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784536164.git.rtoax@foxmail.com?part=2