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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The selftest `test_strncat_success` unconditionally fails because it incorrectly expects a return value of 13, assuming the global buffer retains state across subtests.
- [Medium] Six failure tests for `strcat` and `strncat` will fail to load in the verifier because they pass invalid scalars (`NULL`, `1`, `-1`) to the strictly typed `dst` memory pointer argument.

Pre-existing issues:
- [High] The `__bpf_strncat` kfunc unconditionally performs an out-of-bounds read of the source string to check for truncation, erroneously returning `-EFAULT` on perfectly valid memory.
--

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

selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs

Add tests for new kfuncs bpf_strcat() and bpf_strncat().

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> index bddc4e8579d24..29bc7eabbb6aa 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
[ ... ]
> @@ -53,6 +54,10 @@ SEC("syscall")  __retval(USER_PTR_ERR)int test_strnstr_null1(void *ctx) { return
>  SEC("syscall")  __retval(USER_PTR_ERR)int test_strnstr_null2(void *ctx) { return bpf_strnstr("hello", NULL, 1); }
>  SEC("syscall")  __retval(USER_PTR_ERR)int test_strncasestr_null1(void *ctx) { return bpf_strncasestr(NULL, "hello", 1); }
>  SEC("syscall")  __retval(USER_PTR_ERR)int test_strncasestr_null2(void *ctx) { return bpf_strncasestr("hello", NULL, 1); }
> +SEC("syscall")  __retval(USER_PTR_ERR)int test_strcat_null1(void *ctx) { return bpf_strcat(NULL, 6, "hello"); }

[Severity: Medium]
Will this test, along with test_strcat_user_ptr1 and test_strcat_pagefault1
added below, fail to load in the verifier?

Because the dst argument in bpf_strcat() lacks the __ign suffix, the verifier
strictly checks it as a KF_ARG_PTR_TO_MEM_SIZE. Passing NULL or scalar values
like 1 or -1 causes load-time rejection with -EACCES.

Since the __retval(...) annotation expects a successful load followed by a
runtime error, will these tests cause the test runner to abort with an
unexpected load failure?

> +SEC("syscall")  __retval(USER_PTR_ERR)int test_strcat_null2(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(NULL, 6, "hello", 2); }
> +SEC("syscall")  __retval(USER_PTR_ERR)int test_strncat_null2(void *ctx) { return bpf_strncat(kern_buf, sizeof(kern_buf), NULL, 2); }
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> index f65b1226a81a3..93996b0e9595e 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
[ ... ]
> @@ -59,5 +60,7 @@ __test(-ENOENT) int test_strncasestr_notfound1(void *ctx) { return bpf_strncases
>  __test(-ENOENT) int test_strncasestr_notfound2(void *ctx) { return bpf_strncasestr(str, "hello", 4); }
>  __test(-ENOENT) int test_strncasestr_notfound3(void *ctx) { return bpf_strncasestr("", "a", 0); }
>  __test(0) int test_strncasestr_empty(void *ctx) { return bpf_strncasestr(str, "", 1); }
> +__test(10) int test_strcat_success(void *ctx) { return bpf_strcat(buf, sizeof(buf), "world"); }
> +__test(13) int test_strncat_success(void *ctx) { return bpf_strncat(buf, sizeof(buf), "world", 3); }

[Severity: Medium]
Does this test expect an incorrect return value?

The BPF test loader isolates subtests by reloading the skeleton for each test
program. This resets the global variable buf back to its initial .data state
of "hello" (5 bytes) before this test runs.

Because bpf_strncat() appends "wor" (3 bytes) to "hello", won't the correct
return value be 8 instead of 13?

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the
__bpf_strncat() implementation being tested here.

kernel/bpf/helpers.c:__bpf_strncat() {
        ...
        __get_kernel_nofault(&cs, src, char, err_out);
        if (cs != '\0' && sz > copied)
                return -E2BIG;
        ...
}

Does this unconditionally read past the requested size boundary?

If a caller requests exactly sz bytes, and the source string satisfies this
but ends at a page boundary, won't the unconditional read of the next byte
cause a spurious page fault and incorrectly return -EFAULT instead of success?

>  char _license[] SEC("license") = "GPL";

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