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

[PATCH bpf-next v5 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);
  int bpf_strncat(char *dst, u32 dst__sz, const char *src, 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 | 75 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index c18f1e16edee..85afff60365c 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -4195,6 +4195,79 @@ __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, slen, space, copied;
+
+	if (sz == 0 || dsz == 0)
+		return -EINVAL;
+
+	while (dlen < dsz && dst[dlen] != '\0')
+		dlen++;
+
+	if (dlen >= dsz)
+		return -E2BIG;
+
+	space = dsz - dlen;
+
+	slen = bpf_strnlen(src, sz);
+	if (space <= slen)
+		return -E2BIG;
+
+	copied = strncpy_from_kernel_nofault(dst + dlen, src, sz);
+	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 +5031,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

Re: [PATCH bpf-next v5 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:
- [Critical] Missing negative error check for `bpf_strnlen()` return value bypasses bounds checking and allows arbitrary kernel memory overwrite.
- [Critical] A Time-of-Check to Time-of-Use (TOCTOU) vulnerability allows concurrent modifications of the source string to bypass length checks and cause an out-of-bounds write.
- [High] Using signed `int` for memory offsets and sizes (`dlen` and `space`) can lead to integer overflow and out-of-bounds access for large buffers.
- [Medium] The `src` argument lacks the `__ign` suffix, improperly forcing the BPF verifier to require writable memory for a read-only string parameter.
--

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

bpf: add bpf_strcat,bpf_strncat kfunc

This patch adds string concatenation kfuncs bpf_strcat and bpf_strncat.
These functions differ from glibc by requiring the target string's memory
size 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".

[Severity: Medium]
Is this verifier error happening because the src parameter is missing the
__ign suffix? Functions like bpf_strncasestr use the __ign suffix to tell
the verifier that the pointer is only used for safe probe reads and does
not need to point to writable memory.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee4..85afff60365c8 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4195,6 +4195,79 @@ __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, slen, space, copied;
> +
> +	if (sz == 0 || dsz == 0)
> +		return -EINVAL;
> +
> +	while (dlen < dsz && dst[dlen] != '\0')
> +		dlen++;

[Severity: High]
What happens if dsz is very large?

Since dsz is a u32 and can represent sizes up to 4GB, but dlen is a signed
32-bit integer, could a large buffer from a BPF arena cause dlen to
overflow and become negative? This would cause dst[dlen] to access memory
at a negative offset from the start of the buffer.

> +
> +	if (dlen >= dsz)
> +		return -E2BIG;
> +
> +	space = dsz - dlen;
> +
> +	slen = bpf_strnlen(src, sz);
> +	if (space <= slen)
> +		return -E2BIG;

[Severity: Critical]
What happens if bpf_strnlen returns a negative error code like -E2BIG?

If slen is assigned a negative value and space is a positive integer, won't
the signed comparison space <= slen evaluate to false? This appears to
bypass the bounds check entirely when the source string is missing a null
terminator.

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

[Severity: Critical]
Could this allow an out-of-bounds write if the source string is modified
concurrently?

If the source string is located in a BPF map shared with userspace, could a
userspace thread overwrite the null terminator after bpf_strnlen returns?
If that happens, strncpy_from_kernel_nofault will copy up to sz bytes,
which might be much larger than the remaining space, overflowing the
destination buffer.

Would it be safer to pass the constrained space or slen + 1 instead of sz?

[ ... ]

> +__bpf_kfunc int bpf_strcat(char *dst, u32 dst__sz, const char *src)
> +{
> +	return __bpf_strncat(dst, dst__sz, src, dst__sz);
> +}

[Severity: Medium]
Should the src parameter be named src__ign here to bypass the writability
checks mentioned in the commit message?

[ ... ]

> +__bpf_kfunc int bpf_strncat(char *dst, u32 dst__sz, const char *src, u32 len)
> +{
> +	return __bpf_strncat(dst, dst__sz, src, len);
> +}

[Severity: Medium]
Similarly, does this src parameter also need the __ign suffix to allow
read-only string constants to be passed?

[ ... ]

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

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

[bot+bpf-ci]

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

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee..85afff60365c 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4195,6 +4195,79 @@ __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, slen, space, copied;
> +
> +	if (sz == 0 || dsz == 0)
> +		return -EINVAL;
> +
> +	while (dlen < dsz && dst[dlen] != '\0')
> +		dlen++;
> +
> +	if (dlen >= dsz)
> +		return -E2BIG;
> +
> +	space = dsz - dlen;
> +
> +	slen = bpf_strnlen(src, sz);

This came up in the earlier review of the series: the length of src does not
need to be computed here, since strncpy_from_kernel_nofault() already bounds
the copy by sz.

  https://lore.kernel.org/bpf/3e73aeb2-afce-4ae9-9163-51e4dc1ded01@redhat.com/

That bpf_strnlen(src, sz) call was dropped in an earlier revision and has
returned in this one.  Is the separate src length scan still needed here, or
can strncpy_from_kernel_nofault() handle the bound on its own?

> +	if (space <= slen)
> +		return -E2BIG;
> +
> +	copied = strncpy_from_kernel_nofault(dst + dlen, src, sz);
> +	if (copied < 0)
> +		return copied;
> +	else if (copied == 0 || copied == 1)
> +		return dlen;
> +
> +	/* The copied character count includes '\0'. */
> +	return dlen + copied - 1;
> +}

[ ... ]


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

[PATCH bpf-next v5 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_failure2.c     |  3 +++
 .../selftests/bpf/progs/string_kfuncs_success.c      | 12 ++++++++++++
 3 files changed, 17 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_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..40583361c655 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);
+	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");
+}
 
 char _license[] SEC("license") = "GPL";
-- 
2.55.0

Re: [PATCH bpf-next v5 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_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