[PATCH bpf-next v4 1/2] bpf: add bpf_strcat,bpf_strncat kfunc

[PATCH bpf-next v4 1/2] bpf: add bpf_strcat,bpf_strncat kfunc

[Rong Tao]

From: Rong Tao <rongtao@cestc.cn>

Add string concatenation kfuncs, prototype:

  int bpf_strcat(char *dst, u32 dst__sz, const char *src__ign);
  int bpf_strncat(char *dst, u32 dst__sz, const char *src__ign, u32 len);

This differs from the glibc library functions strcat and strncat, which,
for safety reasons, require the size of the target string's memory space
as a parameter.

The src parameter cannot directly pass a string constant, such as "XYZ",
otherwise it will throw an error "write into map forbidden".

Signed-off-by: Rong Tao <rongtao@cestc.cn>
---
 kernel/bpf/helpers.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index c18f1e16edee..2c09044de311 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -4195,6 +4195,74 @@ __bpf_kfunc int bpf_strncasestr(const char *s1__ign, const char *s2__ign,
 	return __bpf_strnstr(s1__ign, s2__ign, len, true);
 }
 
+static int __bpf_strncat(char *dst, u32 dsz, const char *src, u32 sz)
+{
+	int dlen = 0, space, copied;
+
+	while (dst[dlen] != '\0')
+		dlen++;
+
+	if (dlen >= dsz || sz == 0 || dsz == 0)
+		return -EINVAL;
+
+	space = dsz - dlen;
+	if (space < 1)
+		return -E2BIG;
+
+	copied = strncpy_from_kernel_nofault(dst + dlen, src, min(sz, space));
+	if (copied < 0)
+		return copied;
+	else if (copied == 0 || copied == 1)
+		return dlen;
+
+	/* The copied character count includes '\0'. */
+	return dlen + copied - 1;
+}
+
+/**
+ * bpf_strcat - Append non-null bytes from a source string, and null-terminate
+ *              the result
+ * @dst: Destination string.
+ * @dst__sz: Maximum bytes of @dst__ign, includes the trailing NUL.
+ * @src: Source string.
+ *
+ * Return:
+ * * >=0      - Length of the concatenated string.
+ *
+ * * %-EINVAL - String @dst__ign is invalid.
+ * * %-EFAULT - Cannot read or write one of the strings.
+ * * %-E2BIG  - String @src__ign is too large or the remaining space in
+ *              @dst__ign is too small.
+ * * %-ERANGE - One of the strings is outside of kernel address space
+ */
+__bpf_kfunc int bpf_strcat(char *dst, u32 dst__sz, const char *src)
+{
+	return __bpf_strncat(dst, dst__sz, src, dst__sz);
+}
+
+/**
+ * bpf_strncat - Append non-null bytes from a source string, and null-terminate
+ *               the result
+ * @dst: Destination string.
+ * @dst__sz: Maximum bytes of @dst__ign, includes the trailing NUL.
+ * @src: Source string.
+ * @len: the maximum number of characters to concatenate, the null terminator
+ *       \0 of the target string @dst will also be included.
+ *
+ * Return:
+ * * >=0      - Length of the concatenated string.
+ *
+ * * %-EINVAL - String @dst__ign is invalid.
+ * * %-EFAULT - Cannot read or write one of the strings.
+ * * %-E2BIG  - String @src__ign is too large or the remaining space in
+ *              @dst__ign is too small.
+ * * %-ERANGE - One of the strings is outside of kernel address space
+ */
+__bpf_kfunc int bpf_strncat(char *dst, u32 dst__sz, const char *src, u32 len)
+{
+	return __bpf_strncat(dst, dst__sz, src, len);
+}
+
 #ifdef CONFIG_KEYS
 /**
  * bpf_lookup_user_key - lookup a key by its serial
@@ -4958,6 +5026,8 @@ BTF_ID_FLAGS(func, bpf_strstr);
 BTF_ID_FLAGS(func, bpf_strcasestr);
 BTF_ID_FLAGS(func, bpf_strnstr);
 BTF_ID_FLAGS(func, bpf_strncasestr);
+BTF_ID_FLAGS(func, bpf_strcat);
+BTF_ID_FLAGS(func, bpf_strncat);
 #if defined(CONFIG_BPF_LSM) && defined(CONFIG_CGROUPS)
 BTF_ID_FLAGS(func, bpf_cgroup_read_xattr, KF_RCU)
 #endif
-- 
2.55.0

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

[Rong Tao]

From: Rong Tao <rongtao@cestc.cn>

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

And remove extra whitespaces in selftests string_kfuncs_failure1.c.

Signed-off-by: Rong Tao <rongtao@cestc.cn>
---
 .../selftests/bpf/prog_tests/string_kfuncs.c  |  2 +
 .../bpf/progs/string_kfuncs_failure1.c        | 53 +++++++++++--------
 .../bpf/progs/string_kfuncs_failure2.c        |  3 ++
 .../bpf/progs/string_kfuncs_success.c         | 10 ++++
 4 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/string_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/string_kfuncs.c
index 300032a19445..460567ef622a 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",
 };
 
 void run_too_long_tests(void)
diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
index bddc4e8579d2..d432d4990f80 100644
--- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
+++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
@@ -8,6 +8,7 @@
 
 char *user_ptr = (char *)1;
 char *invalid_kern_ptr = (char *)-1;
+char kern_buf[32] = { "hello" };
 
 /*
  * When passing userspace pointers, the error code differs based on arch:
@@ -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_strcmp_null1(void *ctx) { return bpf_strcmp(NULL, "hello"); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strcmp_null2(void *ctx) { return bpf_strcmp("hello", NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strcmp_null2(void *ctx) { return bpf_strcmp("hello", NULL); }
 SEC("syscall") __retval(USER_PTR_ERR) int test_strcasecmp_null1(void *ctx) { return bpf_strcasecmp(NULL, "HELLO"); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strcasecmp_null2(void *ctx) { return bpf_strcasecmp("HELLO", NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strcasecmp_null2(void *ctx) { return bpf_strcasecmp("HELLO", NULL); }
 SEC("syscall") __retval(USER_PTR_ERR)int test_strncasecmp_null1(void *ctx) { return bpf_strncasecmp(NULL, "HELLO", 5); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strncasecmp_null2(void *ctx) { return bpf_strncasecmp("HELLO", NULL, 5);	 }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strchr_null(void *ctx) { return bpf_strchr(NULL, 'a'); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strchrnul_null(void *ctx) { return bpf_strchrnul(NULL, 'a'); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strnchr_null(void *ctx) { return bpf_strnchr(NULL, 1, 'a'); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strrchr_null(void *ctx) { return bpf_strrchr(NULL, 'a'); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strlen_null(void *ctx) { return bpf_strlen(NULL); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strnlen_null(void *ctx) { return bpf_strnlen(NULL, 1); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strspn_null1(void *ctx) { return bpf_strspn(NULL, "hello"); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strspn_null2(void *ctx) { return bpf_strspn("hello", NULL); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strcspn_null1(void *ctx) { return bpf_strcspn(NULL, "hello"); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strcspn_null2(void *ctx) { return bpf_strcspn("hello", NULL); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strstr_null1(void *ctx) { return bpf_strstr(NULL, "hello"); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strstr_null2(void *ctx) { return bpf_strstr("hello", NULL); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strcasestr_null1(void *ctx) { return bpf_strcasestr(NULL, "hello"); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strcasestr_null2(void *ctx) { return bpf_strcasestr("hello", NULL); }
-SEC("syscall")  __retval(USER_PTR_ERR)int test_strnstr_null1(void *ctx) { return bpf_strnstr(NULL, "hello", 1); }
-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_strncasecmp_null2(void *ctx) { return bpf_strncasecmp("HELLO", NULL, 5);	 }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strchr_null(void *ctx) { return bpf_strchr(NULL, 'a'); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strchrnul_null(void *ctx) { return bpf_strchrnul(NULL, 'a'); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strnchr_null(void *ctx) { return bpf_strnchr(NULL, 1, 'a'); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strrchr_null(void *ctx) { return bpf_strrchr(NULL, 'a'); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strlen_null(void *ctx) { return bpf_strlen(NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strnlen_null(void *ctx) { return bpf_strnlen(NULL, 1); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strspn_null1(void *ctx) { return bpf_strspn(NULL, "hello"); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strspn_null2(void *ctx) { return bpf_strspn("hello", NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strcspn_null1(void *ctx) { return bpf_strcspn(NULL, "hello"); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strcspn_null2(void *ctx) { return bpf_strcspn("hello", NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strstr_null1(void *ctx) { return bpf_strstr(NULL, "hello"); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strstr_null2(void *ctx) { return bpf_strstr("hello", NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strcasestr_null1(void *ctx) { return bpf_strcasestr(NULL, "hello"); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strcasestr_null2(void *ctx) { return bpf_strcasestr("hello", NULL); }
+SEC("syscall") __retval(USER_PTR_ERR)int test_strnstr_null1(void *ctx) { return bpf_strnstr(NULL, "hello", 1); }
+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(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_strcmp_user_ptr1(void *ctx) { return bpf_strcmp(user_ptr, "hello"); }
@@ -79,6 +82,10 @@ SEC("syscall") __retval(USER_PTR_ERR) int test_strnstr_user_ptr1(void *ctx) { re
 SEC("syscall") __retval(USER_PTR_ERR) int test_strnstr_user_ptr2(void *ctx) { return bpf_strnstr("hello", user_ptr, 1); }
 SEC("syscall") __retval(USER_PTR_ERR) int test_strncasestr_user_ptr1(void *ctx) { return bpf_strncasestr(user_ptr, "hello", 1); }
 SEC("syscall") __retval(USER_PTR_ERR) int test_strncasestr_user_ptr2(void *ctx) { return bpf_strncasestr("hello", user_ptr, 1); }
+SEC("syscall") __retval(USER_PTR_ERR) int test_strcat_user_ptr1(void *ctx) { return bpf_strcat(user_ptr, 1, kern_buf); }
+SEC("syscall") __retval(USER_PTR_ERR) int test_strcat_user_ptr2(void *ctx) { return bpf_strcat(kern_buf, sizeof(kern_buf), user_ptr); }
+SEC("syscall") __retval(USER_PTR_ERR) int test_strncat_user_ptr1(void *ctx) { return bpf_strncat(user_ptr, 1, kern_buf, 2); }
+SEC("syscall") __retval(USER_PTR_ERR) int test_strncat_user_ptr2(void *ctx) { return bpf_strncat(kern_buf, sizeof(kern_buf), user_ptr, 1); }
 
 #endif /* __TARGET_ARCH_s390 */
 
@@ -107,5 +114,9 @@ SEC("syscall") __retval(-EFAULT) int test_strnstr_pagefault1(void *ctx) { return
 SEC("syscall") __retval(-EFAULT) int test_strnstr_pagefault2(void *ctx) { return bpf_strnstr("hello", invalid_kern_ptr, 1); }
 SEC("syscall") __retval(-EFAULT) int test_strncasestr_pagefault1(void *ctx) { return bpf_strncasestr(invalid_kern_ptr, "hello", 1); }
 SEC("syscall") __retval(-EFAULT) int test_strncasestr_pagefault2(void *ctx) { return bpf_strncasestr("hello", invalid_kern_ptr, 1); }
+SEC("syscall") __retval(-EFAULT) int test_strcat_pagefault1(void *ctx) { return bpf_strcat(invalid_kern_ptr, 1, kern_buf); }
+SEC("syscall") __retval(-EFAULT) int test_strcat_pagefault2(void *ctx) { return bpf_strcat(kern_buf, sizeof(kern_buf), invalid_kern_ptr); }
+SEC("syscall") __retval(-EFAULT) int test_strncat_pagefault1(void *ctx) { return bpf_strncat(invalid_kern_ptr, 1, kern_buf, 2); }
+SEC("syscall") __retval(-EFAULT) int test_strncat_pagefault2(void *ctx) { return bpf_strncat(kern_buf, sizeof(kern_buf), invalid_kern_ptr, 2); }
 
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
index 412c53b87b18..2dbd1349689e 100644
--- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
+++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
@@ -5,6 +5,7 @@
 #include <linux/limits.h>
 
 char long_str[XATTR_SIZE_MAX + 1];
+char str[] = "hello";
 
 SEC("syscall") int test_strcmp_too_long(void *ctx) { return bpf_strcmp(long_str, long_str); }
 SEC("syscall") int test_strcasecmp_too_long(void *ctx) { return bpf_strcasecmp(long_str, long_str); }
@@ -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); }
 
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
index f65b1226a81a..71fbbe97b4c5 100644
--- a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
+++ b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
@@ -59,5 +59,15 @@ __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);
+	return bpf_strcmp(buf, "hellohello world");
+}
+__test(8) int test_strncat_success(void *ctx) {
+	char buf[32] = "hello";
+	bpf_strncat(buf, sizeof(buf), str, 3);
+	return bpf_strcmp(buf, "hellohe");
+}
 
 char _license[] SEC("license") = "GPL";
-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH bpf-next v4 1/2] bpf: add bpf_strcat,bpf_strncat kfunc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The while (dst[dlen] != '\0') loop in __bpf_strncat lacks a bounds check against dsz, allowing out-of-bounds reads and potential kernel panics.
- [High] The src argument in bpf_strcat and bpf_strncat lacks a __str suffix or size parameter, causing the verifier to only guarantee 1 byte of memory safety, allowing strncpy_from_kernel_nofault to leak adjacent kernel memory.
- [Medium] Silent string truncation occurs without returning -E2BIG as documented, and the space < 1 check is mathematically unreachable.
- [Low] Incompatible types in strict min() macro trigger compile-time warnings/errors due to signedness mismatch.
--

--- Patch [1]: [PATCH bpf-next v4 1/2] bpf: add bpf_strcat,bpf_strncat kfunc ---
commit e18a916b95792b0a70aea4ebf35e2830d909b3ee
Author: Rong Tao <rongtao@cestc.cn>

bpf: add bpf_strcat,bpf_strncat kfunc

This commit adds string concatenation kfuncs bpf_strcat and bpf_strncat.
These differ from standard glibc functions by requiring the destination size
as a parameter for safety.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee4..2c09044de3112 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4195,6 +4195,74 @@ __bpf_kfunc int bpf_strncasestr(const char *s1__ign, const char *s2__ign,
>  	return __bpf_strnstr(s1__ign, s2__ign, len, true);
>  }
>  
> +static int __bpf_strncat(char *dst, u32 dsz, const char *src, u32 sz)
> +{
> +	int dlen = 0, space, copied;
> +
> +	while (dst[dlen] != '\0')
> +		dlen++;

[Severity: High]
Does this loop need a bounds check against dsz?  If a BPF program passes a
buffer for dst that does not contain a null terminator, this loop will
increment dlen indefinitely and read past the end of the verified buffer.

> +
> +	if (dlen >= dsz || sz == 0 || dsz == 0)
> +		return -EINVAL;
> +
> +	space = dsz - dlen;
> +	if (space < 1)
> +		return -E2BIG;

[Severity: Medium]
Since dlen >= dsz is checked right before this, space = dsz - dlen is
guaranteed to be >= 1. Doesn't that make this space < 1 check dead code?

Also, if the remaining space is too small for the source string, the code
below silently truncates instead of returning -E2BIG as documented. Should it
return an error instead?

> +
> +	copied = strncpy_from_kernel_nofault(dst + dlen, src, min(sz, space));

[Severity: Low]
Will this trigger a build error? The min() macro requires arguments of
compatible types, but sz is u32 (unsigned) and space is int (signed).

> +	if (copied < 0)
> +		return copied;
> +	else if (copied == 0 || copied == 1)
> +		return dlen;
> +
> +	/* The copied character count includes '\0'. */
> +	return dlen + copied - 1;
> +}

[ ... ]

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

[Severity: High]
Without a __str or __sz suffix, won't the verifier treat const char *src as a
pointer to a single byte?

A BPF program could pass a pointer to a 1-byte map value, and __bpf_strncat()
could then use strncpy_from_kernel_nofault() to read up to dst__sz bytes,
potentially leaking out-of-bounds kernel memory into dst.

> +{
> +	return __bpf_strncat(dst, dst__sz, src, dst__sz);
> +}

[ ... ]

> +__bpf_kfunc int bpf_strncat(char *dst, u32 dst__sz, const char *src, u32 len)

[Severity: High]
Does this src parameter also need a __str suffix or a src__sz parameter to
prevent the verifier from treating it as a 1-byte buffer?

> +{
> +	return __bpf_strncat(dst, dst__sz, src, len);
> +}

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

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

[bot+bpf-ci]

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

> selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs
>
> Add tests for new kfuncs bpf_strcat() and bpf_strncat().
>
> And remove extra whitespaces in selftests string_kfuncs_failure1.c.
>
> Signed-off-by: Rong Tao <rongtao@cestc.cn>

This isn't a bug, but could the whitespace cleanup in
string_kfuncs_failure1.c be split into its own patch rather than folded
into the commit that adds the strcat/strncat tests?  The commit message
already describes them as two separate changes, and the whitespace-only
churn ends up mixed in with the new test cases:

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> index bddc4e8579d2..d432d4990f80 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_strcmp_null1(void *ctx) { return bpf_strcmp(NULL, "hello"); }
> -SEC("syscall")  __retval(USER_PTR_ERR)int test_strcmp_null2(void *ctx) { return bpf_strcmp("hello", NULL); }
> +SEC("syscall") __retval(USER_PTR_ERR)int test_strcmp_null2(void *ctx) { return bpf_strcmp("hello", NULL); }
>  SEC("syscall") __retval(USER_PTR_ERR) int test_strcasecmp_null1(void *ctx) { return bpf_strcasecmp(NULL, "HELLO"); }
> -SEC("syscall")  __retval(USER_PTR_ERR)int test_strcasecmp_null2(void *ctx) { return bpf_strcasecmp("HELLO", NULL); }
> +SEC("syscall") __retval(USER_PTR_ERR)int test_strcasecmp_null2(void *ctx) { return bpf_strcasecmp("HELLO", NULL); }

[ ... ]

> +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); }

Keeping the two apart would make the new test additions easier to read on
their own.


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