Re: [PATCH bpf-next v5 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_strcat and bpf_strncat are vulnerable to an out-of-bounds write (buffer overflow) via a Time-of-Check to Time-of-Use (TOCTOU) race condition when concatenating from a concurrently modified source.
- [Critical] Out-of-bounds write (buffer overflow) in `__bpf_strncat` due to mishandling of negative error codes from `bpf_strnlen` when the source string lacks a null terminator.
- [High] bpf_strcat and bpf_strncat allow reading beyond the bounds of the memory object pointed to by src, resulting in an information leak of adjacent kernel memory.
- [High] bpf_strncat unexpectedly truncates the appended string by 1 character if the source string length is equal to or exceeds the specified limit.
- [Medium] The selftests for bpf_strcat and bpf_strncat do not verify the behavior when the source string is too long or lacks a null terminator.
--

--- Patch [2]: [PATCH bpf-next v5 2/2] selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs ---
commit 4c36dbcb540f828a7c99805567e0566a0ff526c6
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_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); }
> +SEC("syscall") int test_strncat_too_long(void *ctx) { return bpf_strncat(long_str, sizeof(long_str), str, 3); }

[Severity: Medium]
Is this test effectively validating the behavior of a malformed source string?

By passing long_str as the destination buffer, the kfunc immediately fails
the destination length check (dlen >= dsz) and returns -E2BIG.

This bypasses the core string concatenation logic and leaves the handling of
source strings that are too long or lacking a null terminator entirely
untested.

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> index f65b1226a81a3..40583361c655c 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c

[ ... ]

> @@ -59,5 +59,17 @@ __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(0) int test_strcat_success(void *ctx)
> +{
> +	char buf[32] = "hello";
> +	bpf_strcat(buf, sizeof(buf), str);

[Severity: High]
Could this signature allow an information leak of adjacent kernel memory?

The kfunc signature in kernel/bpf/helpers.c defines the source string
without a size suffix or __str annotation:

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

Because of this, the verifier only guarantees 1 byte of memory safety for
the source buffer. However, __bpf_strncat reads up to dst__sz bytes from it.

If the object the source points to is smaller than dst__sz and adjacent to
other kernel data, couldn't this read past the object's bounds and expose
the adjacent data in the concatenated result?

[Severity: Critical]
Is there a Time-of-Check to Time-of-Use race condition here that can result
in a buffer overflow?

In __bpf_strncat, after checking the length with bpf_strnlen, the copy is
performed using the full input limit sz instead of the remaining space in
the destination buffer:

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

If the source string is concurrently modified to remove the null terminator
after the length check passes, strncpy_from_kernel_nofault will read up to
sz bytes.

Since it writes to dst + dlen, the total written extent would be
dst + dlen + sz, which overflows dst__sz by dlen bytes.

[Severity: Critical]
How does __bpf_strncat handle negative error codes from bpf_strnlen?

When the source string is not null-terminated within the limit, bpf_strnlen
returns -E2BIG (-7). This negative value is assigned to the signed integer
slen.

The bounds check then evaluates to false:

kernel/bpf/helpers.c:__bpf_strncat() {
    ...
    if (space <= slen)
        return -E2BIG;
    ...
}

For example, 65526 <= -7 is false. The function then proceeds to call
strncpy_from_kernel_nofault with sz, copying up to sz bytes into
dst + dlen and overflowing the buffer.

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

[Severity: High]
Does this hardcoded expectation reveal a truncation bug in bpf_strncat?

This test expects bpf_strncat with a limit of 3 to append only 2 characters
("he"). Standard strncat(dst, src, len) appends up to len characters and
then adds a null byte, which would result in 3 characters appended.

Looking at __bpf_strncat, it calls:

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

If no null byte is found within sz bytes of the source,
strncpy_from_kernel_nofault copies sz bytes but overwrites the last copied
byte with \0. This inherently truncates the result to sz - 1 characters,
diverging from standard strncat semantics.

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

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