Re: [PATCH bpf-next v4 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] The while (dst[dlen] != '\0') loop in __bpf_strncat lacks a bounds check against dsz, allowing out-of-bounds reads and potential kernel panics.
- [High] The src argument in bpf_strcat and bpf_strncat lacks a __str suffix or size parameter, causing the verifier to only guarantee 1 byte of memory safety, allowing strncpy_from_kernel_nofault to leak adjacent kernel memory.
- [Medium] Silent string truncation occurs without returning -E2BIG as documented, and the space < 1 check is mathematically unreachable.
- [Low] Incompatible types in strict min() macro trigger compile-time warnings/errors due to signedness mismatch.
--

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

bpf: add bpf_strcat,bpf_strncat kfunc

This commit adds string concatenation kfuncs bpf_strcat and bpf_strncat.
These differ from standard glibc functions by requiring the destination size
as a parameter for safety.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee4..2c09044de3112 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4195,6 +4195,74 @@ __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, space, copied;
> +
> +	while (dst[dlen] != '\0')
> +		dlen++;

[Severity: High]
Does this loop need a bounds check against dsz?  If a BPF program passes a
buffer for dst that does not contain a null terminator, this loop will
increment dlen indefinitely and read past the end of the verified buffer.

> +
> +	if (dlen >= dsz || sz == 0 || dsz == 0)
> +		return -EINVAL;
> +
> +	space = dsz - dlen;
> +	if (space < 1)
> +		return -E2BIG;

[Severity: Medium]
Since dlen >= dsz is checked right before this, space = dsz - dlen is
guaranteed to be >= 1. Doesn't that make this space < 1 check dead code?

Also, if the remaining space is too small for the source string, the code
below silently truncates instead of returning -E2BIG as documented. Should it
return an error instead?

> +
> +	copied = strncpy_from_kernel_nofault(dst + dlen, src, min(sz, space));

[Severity: Low]
Will this trigger a build error? The min() macro requires arguments of
compatible types, but sz is u32 (unsigned) and space is int (signed).

> +	if (copied < 0)
> +		return copied;
> +	else if (copied == 0 || copied == 1)
> +		return dlen;
> +
> +	/* The copied character count includes '\0'. */
> +	return dlen + copied - 1;
> +}

[ ... ]

> +__bpf_kfunc int bpf_strcat(char *dst, u32 dst__sz, const char *src)

[Severity: High]
Without a __str or __sz suffix, won't the verifier treat const char *src as a
pointer to a single byte?

A BPF program could pass a pointer to a 1-byte map value, and __bpf_strncat()
could then use strncpy_from_kernel_nofault() to read up to dst__sz bytes,
potentially leaking out-of-bounds kernel memory into dst.

> +{
> +	return __bpf_strncat(dst, dst__sz, src, dst__sz);
> +}

[ ... ]

> +__bpf_kfunc int bpf_strncat(char *dst, u32 dst__sz, const char *src, u32 len)

[Severity: High]
Does this src parameter also need a __str suffix or a src__sz parameter to
prevent the verifier from treating it as a 1-byte buffer?

> +{
> +	return __bpf_strncat(dst, dst__sz, src, len);
> +}

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