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

[sashiko-bot@kernel.org]

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