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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Arbitrary Kernel Memory Write via Verifier Bypass
- [High] Missing page fault protection leading to sleeping in atomic context
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race leading to unbounded loop and soft lockup
- [Medium] Unbounded out-of-bounds read on destination buffer
- [Medium] Unbounded out-of-bounds read on source buffer
--

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

bpf: add bpf_strcat,bpf_strncat kfunc

This commit introduces bpf_strcat and bpf_strncat kfuncs to allow string
concatenation in BPF programs. It requires the destination buffer size as a
parameter 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);

[Severity: Medium]
Does this code perform an unbounded read on the destination buffer? The limit
should be bounded by the provided size dsz, rather than statically set to
XATTR_SIZE_MAX.

> +	if (dlen < 0)
> +		return dlen;
> +	slen = bpf_strnlen(src, XATTR_SIZE_MAX);

[Severity: Medium]
Does this scan read out-of-bounds on the source buffer? The limit could be
bounded by the requested copy length sz rather than unconditionally reading
up to XATTR_SIZE_MAX.

> +	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++) {

[Severity: High]
Does this code introduce a Time-Of-Check to Time-Of-Use race? The loop bounds
omit checking copied < slen, relying entirely on encountering a null
terminator.

If a concurrent thread modifies the source buffer to remove the null terminator
after bpf_strnlen() executes, could this loop run out of bounds and lead to a
soft lockup?

> +		__get_kernel_nofault(&cs, src, char, err_out);

[Severity: High]
Does this sequence lack page fault protection? Unlike other string helpers,
__bpf_strncat() fails to wrap its memory accesses in guard(pagefault)().

If a BPF program executing in a non-sleepable context calls this on an unmapped
address, it seems the generic page fault handler will attempt to take sleepable
locks.

> +		if (cs == '\0')
> +			break;
> +
> +		__put_kernel_nofault(dst + dlen + copied, &cs, char, err_out);

[Severity: Critical]
Can this write to arbitrary kernel memory? The function writes to the unchecked
destination pointer.

Because the bpf_strcat() kfunc signature uses the __ign suffix for dst__ign,
verifier safety checks are bypassed. Any user capable of loading BPF programs
could potentially use this to write user-controlled bytes to arbitrary kernel
memory.

> +
> +		src++;
> +	}
[ ... ]
> +__bpf_kfunc int bpf_strcat(char *dst__ign, u32 dst__sz, const char *src__ign)

[Severity: Critical]
Does the dst__ign argument bypass verifier checks? The __ign suffix explicitly
instructs the BPF verifier to ignore this argument, which removes all memory
safety and bounds checks for the destination pointer.

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