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

[PATCH bpf-next v3 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.

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

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index c18f1e16edee..e4708a4f3470 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -4195,6 +4195,97 @@ __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, slen, space, copied;
+	char cs;
+
+	if (!copy_from_kernel_nofault_allowed(dst, 1) ||
+	    !copy_from_kernel_nofault_allowed(src, 1)) {
+		return -ERANGE;
+	}
+
+	dlen = bpf_strnlen(dst, dsz);
+	if (dlen < 0)
+		return dlen;
+	slen = bpf_strnlen(src, sz);
+	if (slen < 0)
+		return slen;
+
+	if (dlen >= dsz || sz == 0 || dsz == 0)
+		return -EINVAL;
+
+	space = dsz - dlen;
+	if (space <= 1 || space < min(slen, sz) + 1)
+		return -E2BIG;
+
+	guard(pagefault)();
+
+	copied = strncpy_from_kernel_nofault(dst + dlen, src,
+				      min(space, sz + 1));
+	if (copied < 0)
+		return copied;
+	else if (copied == 0 || copied == 1)
+		return dlen;
+
+	/* The copied character count includes '\0'. */
+	copied--;
+
+	if (copied < sz) {
+		__get_kernel_nofault(&cs, src + copied, char, err_out);
+		if (cs != '\0' && sz > copied)
+			return -E2BIG;
+	}
+
+	return dlen + copied;
+err_out:
+	return -EFAULT;
+}
+
+/**
+ * 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__ign: 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__ign)
+{
+	return __bpf_strncat(dst, dst__sz, src__ign, XATTR_SIZE_MAX);
+}
+
+/**
+ * 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__ign: Source string.
+ * @len: the maximum number of characters to concatenate
+ *
+ * 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__ign,
+			    u32 len)
+{
+	return __bpf_strncat(dst, dst__sz, src__ign, len);
+}
+
 #ifdef CONFIG_KEYS
 /**
  * bpf_lookup_user_key - lookup a key by its serial
@@ -4958,6 +5049,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 v3 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().

Signed-off-by: Rong Tao <rongtao@cestc.cn>
---
 .../testing/selftests/bpf/prog_tests/string_kfuncs.c  |  2 ++
 .../selftests/bpf/progs/string_kfuncs_failure1.c      | 11 +++++++++++
 .../selftests/bpf/progs/string_kfuncs_failure2.c      |  2 ++
 .../selftests/bpf/progs/string_kfuncs_success.c       |  4 ++++
 4 files changed, 19 insertions(+)

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..973a6ba643d4 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:
@@ -53,6 +54,8 @@ 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(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, "hello"); }
+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, "hello", 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, "hello"); }
+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, "hello", 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..38f0a5f326b5 100644
--- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
+++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure2.c
@@ -23,5 +23,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), "hello"); }
+SEC("syscall") int test_strncat_too_long(void *ctx) { return bpf_strncat(long_str, sizeof(long_str), "hello", 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..b24456ad3d43 100644
--- a/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
+++ b/tools/testing/selftests/bpf/progs/string_kfuncs_success.c
@@ -6,6 +6,8 @@
 #include "errno.h"
 
 char str[] = "hello world";
+char buf1[32] = "hello";
+char buf2[32] = "hello";
 
 #define __test(retval) SEC("syscall") __success __retval(retval)
 
@@ -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); }
 
 char _license[] SEC("license") = "GPL";
-- 
2.55.0

Re: [PATCH bpf-next v3 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] bpf_strncat performs a 1-byte out-of-bounds read on the source string when it is not null-terminated within the requested length, causing a spurious -EFAULT.
- [High] The bpf_strcat and bpf_strncat kfuncs use the __ign suffix for the src argument, bypassing BPF verifier bounds checks and allowing an out-of-bounds kernel memory leak.
- [Medium] An integer overflow in the size calculation causes bpf_strncat to silently fail to concatenate data when len is U32_MAX.
- [Medium] bpf_strncat incorrectly returns -E2BIG when appending an empty string to a destination buffer that has exactly 1 byte of space left.
--

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

bpf: add bpf_strcat,bpf_strncat kfunc

This commit introduces string concatenation kfuncs, bpf_strcat and
bpf_strncat, which take the destination buffer size as a parameter for safety.
It differs from the glibc library functions to ensure bounded string operations
in BPF programs.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee4..e4708a4f3470a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
[ ... ]
> +static int __bpf_strncat(char *dst, u32 dsz, const char *src, u32 sz)
> +{
> +	int dlen, slen, space, copied;
> +	char cs;
[ ... ]
> +	space = dsz - dlen;
> +	if (space <= 1 || space < min(slen, sz) + 1)
> +		return -E2BIG;

[Severity: Medium]
Does this incorrectly reject appending an empty string when the destination
buffer is exactly full?

If a BPF program attempts to append an empty string and space == 1 (leaving
just enough room for the existing null terminator), the space <= 1 condition
evaluates to true and unconditionally returns -E2BIG instead of successfully
returning the original length.

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

[Severity: High]
Can this perform an out-of-bounds read on the source string?

If a BPF program calls bpf_strncat() with a sz parameter that exactly
matches the size of a source buffer lacking a null terminator, the expression
sz + 1 forces strncpy_from_kernel_nofault() to read one byte past the
requested length. This could trigger a spurious -EFAULT if the buffer ends at
a page boundary.

[Severity: Medium]
Also, does the size calculation suffer from an integer overflow?

If a BPF program passes U32_MAX for the sz argument to append up to the
maximum possible string length, the expression sz + 1 overflows to 0 in
32-bit unsigned arithmetic. This results in strncpy_from_kernel_nofault()
being called with a copy count of 0, returning success without actually
appending any data.

> +	if (copied < 0)
> +		return copied;
[ ... ]
> +/**
> + * bpf_strcat - Append non-null bytes from a source string, and null-terminate
> + *              the result
[ ... ]
> + */
> +__bpf_kfunc int bpf_strcat(char *dst, u32 dst__sz, const char *src__ign)
> +{
> +	return __bpf_strncat(dst, dst__sz, src__ign, XATTR_SIZE_MAX);
> +}

[Severity: High]
Does the __ign suffix on the source string argument bypass BPF verifier
bounds checks?

The __ign suffix tells the verifier to ignore size constraints that normally
apply to memory pointers. A BPF program could pass a pointer to a small
allocated region (like a 4-byte map value) along with a large len parameter,
allowing strncpy_from_kernel_nofault() to read past the buffer into adjacent
kernel memory until a null byte is encountered.

This would leak out-of-bounds kernel memory into the user-accessible dst
buffer.

> +
> +/**
> + * bpf_strncat - Append non-null bytes from a source string, and null-terminate
> + *               the result
[ ... ]
> + */
> +__bpf_kfunc int bpf_strncat(char *dst, u32 dst__sz, const char *src__ign,
> +			    u32 len)
> +{
> +	return __bpf_strncat(dst, dst__sz, src__ign, len);
> +}

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

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

[sashiko-bot]

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