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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf_strncat unconditionally reads one byte beyond the caller-specified source limit (len) after the copy loop, causing a spurious -EFAULT if the source buffer ends at a page boundary.
- [Medium] The destination string is left completely non-null-terminated if a memory fault occurs during the copy loop.
- [Medium] Integer overflow in the `space` calculation causes spurious `-E2BIG` failures for destination buffers larger than 2GB.
--

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

bpf: add bpf_strcat,bpf_strncat kfunc

This patch adds string concatenation kfuncs bpf_strcat and bpf_strncat to
the BPF subsystem. These functions allow appending non-null bytes from a
source string to a destination string with size limits.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee4..401f94efd687c 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
[ ... ]
> @@ -4195,6 +4195,96 @@ __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, dsz);
> +	if (dlen < 0)
> +		return dlen;
> +	slen = bpf_strnlen(src, sz);
> +	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;

[Severity: Medium]
Will this calculation overflow if dsz is greater than or equal to 2GB?

Since space is a signed int and dsz is a u32, dsz - dlen can result
in a negative number for large destination buffers, such as those from a
BPF arena. A negative value satisfies the space <= 1 check, causing the
function to incorrectly abort and return -E2BIG.

> +
> +	guard(pagefault)();
> +	for (copied = 0; copied < space - 1 && copied < slen; copied++) {
> +		__get_kernel_nofault(&cs, src, char, err_out);
> +		if (cs == '\0')
> +			break;
> +
> +		__put_kernel_nofault(dst + dlen + copied, &cs, char, err_out);

[Severity: Medium]
Does this leave the destination string non-null-terminated if a memory fault
occurs during the copy?

If the loop copies at least one character (overwriting the destination's
original null terminator) and then a subsequent read or write triggers a
page fault, the code jumps directly to err_out without restoring a null
terminator.

While it returns an error, BPF programs that do not check the return value
might operate on corrupted state.

> +
> +		src++;
> +	}
> +	cs = '\0';
> +	__put_kernel_nofault(dst + dlen + copied, &cs, char, err_out);
> +
> +	__get_kernel_nofault(&cs, src, char, err_out);
> +	if (cs != '\0' && sz > copied)
> +		return -E2BIG;

[Severity: High]
Can this unconditionally read one byte beyond the caller-specified limit sz?

When the copy loop finishes, copied may equal sz. The pointer src will
have been incremented sz times. If the original src buffer is exactly
sz bytes and ends at a page boundary, this read will trigger a page fault,
causing a spurious -EFAULT return.

Additionally, if copied == sz, the subsequent sz > copied check evaluates
to false anyway, making the read unnecessary in that case.

> +
> +	return dlen + copied;
> +err_out:
> +	return -EFAULT;
> +}
[ ... ]

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