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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `__bpf_strncat` reads `sz + 1` bytes from `src` when no null terminator is found within the first `sz` bytes, violating bounded string semantics and potentially causing spurious `-EFAULT` errors.
- [High] When `sz` is `U32_MAX`, the expression `sz + 1` overflows to `0` in `__bpf_strncat`, causing it to append zero bytes and silently fail to perform the concatenation.
--

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

selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs

This commit adds selftests for the newly introduced bpf_strcat() and
bpf_strncat() kfuncs. It includes various test cases verifying successful
operations, error handling for NULL pointers and invalid memory accesses, and
buffer overflow conditions.

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> index f65b1226a81a3..b24456ad3d43b 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
[ ... ]
> @@ -59,5 +61,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(buf1, sizeof(buf1), "world"); }
> +__test(8) int test_strncat_success(void *ctx) { return bpf_strncat(buf2, sizeof(buf2), "world", 3); }

[Severity: High]
While reviewing these tests, looking at the __bpf_strncat() implementation
added earlier in this series, could this result in an integer overflow if a
BPF program passes U32_MAX for the len argument (sz)?

kernel/bpf/helpers.c:__bpf_strncat() {
    ...
	copied = strncpy_from_kernel_nofault(dst + dlen, src,
				      min(space, sz + 1));
    ...
}

When sz is U32_MAX, sz + 1 overflows to 0, causing the copy routine to
append zero bytes and silently fail to perform the concatenation, instead of
returning an error.

[Severity: High]
Also looking at the same __bpf_strncat() implementation, can this cause a
spurious -EFAULT if the source string exactly aligns with the end of a mapped
memory page and does not contain a null terminator within the first sz bytes?

Because the copy routine evaluates sz + 1, it unconditionally attempts to
read one byte past the sz boundary specified by the user if no null
terminator is found earlier.

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