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