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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The `bpf_strcat` and `bpf_strncat` kfuncs use the `__ign` suffix for their destination pointer arguments, bypassing the BPF verifier's safety checks and creating an arbitrary kernel memory write vulnerability.
- [Medium] `__bpf_strncat` performs out-of-bounds reads on the source string by ignoring the user-provided limit during length calculation and unconditionally fetching an extra byte after the copy loop.
- [Low] The API documentation for `bpf_strcat` and `bpf_strncat` incorrectly states they return the total length of the concatenated string.
--

--- Patch [2]: [PATCH bpf-next 2/2] selftests/bpf: Test bpf_strcat,bpf_strncat kfuncs ---
commit a7b2e5fa67dde4d93faf939b8283a2aa59b8ca2d
Author: Rong Tao <rongtao@cestc.cn>

bpf: add bpf_strcat,bpf_strncat kfunc

Add string concatenation kfuncs bpf_strcat and bpf_strncat. This allows
BPF programs to concatenate strings, differing from glibc by requiring
the target string's memory size for safety.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee4..e99b643136822 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -4195,6 +4195,95 @@ __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, XATTR_SIZE_MAX);
> +	if (dlen < 0)
> +		return dlen;
> +	slen = bpf_strnlen(src, XATTR_SIZE_MAX);

[Severity: Medium]
Does this calculate the length of the source string past the user-provided
limit sz?

If src is exactly sz bytes long and not null-terminated, won't this read
out of bounds up to XATTR_SIZE_MAX and potentially return -EFAULT?

> +	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;
> +
> +	for (copied = 0; copied < space - 1 && copied < sz; copied++) {
> +		__get_kernel_nofault(&cs, src, char, err_out);
> +		if (cs == '\0')
> +			break;
> +
> +		__put_kernel_nofault(dst + dlen + copied, &cs, char, err_out);
> +
> +		src++;
> +	}
> +	cs = '\0';
> +	__put_kernel_nofault(dst + dlen + copied, &cs, char, err_out);
> +
> +	__get_kernel_nofault(&cs, src, char, err_out);

[Severity: Medium]
If the copy loop ends because copied == sz, does this unconditional read
fetch an extra byte from the advanced src pointer?

Could this cause an out-of-bounds read and a false -EFAULT failure?

> +	if (cs != '\0' && sz > copied)
> +		return -E2BIG;
> +
> +	return copied;
> +err_out:
> +	return -EFAULT;
> +}
> +
> +/**
> + * bpf_strcat - Append non-null bytes from a source string, and null-terminate
> + *              the result
> + * @dst__ign: Destination string.
> + * @dst__sz: Maximum bytes of @dst__ign, includes the trailing NUL.
> + * @src__ign: Source string.
> + *
> + * Return:
> + * * >=0      - Length of the concatenated string.

[Severity: Low]
Is this documentation misleading?

It claims to return the length of the concatenated string, but __bpf_strncat
returns copied, which is the number of newly appended characters.

> + *
> + * * %-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__ign, u32 dst__sz, const char *src__ign)

[Severity: Critical]
Does the __ign suffix on dst__ign bypass the BPF verifier's safety checks?

Since is_kfunc_arg_ignore() in kernel/bpf/verifier.c skips validation for
arguments with the __ign suffix, can a BPF program pass an arbitrary kernel
address (like modprobe_path) instead of a validated PTR_TO_MEM, allowing an
arbitrary kernel memory write?

> +{
> +	return __bpf_strncat(dst__ign, dst__sz, src__ign, XATTR_SIZE_MAX);
> +}
> +
> +/**
> + * bpf_strncat - Append non-null bytes from a source string, and null-terminate
> + *               the result
> + * @dst__ign: 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__ign, u32 dst__sz, const char *src__ign,
> +			    u32 len)

[Severity: Critical]
Does the dst__ign parameter here also bypass verifier safety checks for the
same reason, allowing arbitrary memory writes?

> +{
> +	return __bpf_strncat(dst__ign, dst__sz, src__ign, len);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784021302.git.rongtao@cestc.cn?part=2