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

[Viktor Malik <vmalik@redhat.com>]

On 7/16/26 11:34, Rong Tao wrote:
> From: Rong Tao <rongtao@cestc.cn>
> 
> Add string concatenation kfuncs, prototype:
> 
>   int bpf_strcat(char *dst, u32 dst__sz, const char *src__ign);
>   int bpf_strncat(char *dst, u32 dst__sz, const char *src__ign, u32 len);

Hi,

while I'm still not 100% sure that we need these since the same can be
achieved with bpf_probe_read_kernel_str, I do see some value in
ergonomics.

I have quite a few comments, though. See below.

> 
> This differs from the glibc library functions strcat and strncat, which,
> for safety reasons, require the size of the target string's memory space
> as a parameter.
> 
> Signed-off-by: Rong Tao <rongtao@cestc.cn>
> ---
>  kernel/bpf/helpers.c | 93 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index c18f1e16edee..e4708a4f3470 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4195,6 +4195,97 @@ __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) ||

We're not copying anything from dst, this check is not necessary.

> +	    !copy_from_kernel_nofault_allowed(src, 1)) {

This is checked by strncpy_from_kernel_nofault already, no need for a
duplicate check IMHO.

> +		return -ERANGE;
> +	}
> +
> +	dlen = bpf_strnlen(dst, dsz);

Since dst is not a pointer to unsafe kernel memory, the verifier should
make sure that the memory is acessible. No need for bpf_strlen here, we
can just search for the null byte directly.

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

I don't think we need to know the size of src at all here.
strncpy_from_kernel_nofault will handle it.

> +	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;
> +
> +	guard(pagefault)();

strncpy_from_kernel_nofault already protects from pagefaults.

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

Sashiko made some good points about the `sz + 1` here. Could we just use
`sz` and add the null terminator manually afterwards?

> +	if (copied < 0)
> +		return copied;
> +	else if (copied == 0 || copied == 1)
> +		return dlen;
> +
> +	/* The copied character count includes '\0'. */
> +	copied--;
> +
> +	if (copied < sz) {
> +		__get_kernel_nofault(&cs, src + copied, char, err_out);
> +		if (cs != '\0' && sz > copied)

`sz > copied` is trivially true here since we're in the `copied < sz`
branch.

> +			return -E2BIG;

I don't think this is correct behavior. If this is called via
bpf_strncat, then the last copied byte of src may not be '\0' but we
don't want to return -E2BIG.

In fact, I don't think we need the explicit bounding of bpf_strcat to
XATTR_SIZE_MAX (as we do with other string kfuncs) because we are
implicitly bounded by the size of dst.

Viktor

> +	}
> +
> +	return dlen + copied;
> +err_out:
> +	return -EFAULT;
> +}
> +
> +/**
> + * bpf_strcat - Append non-null bytes from a source string, and null-terminate
> + *              the result
> + * @dst: Destination string.
> + * @dst__sz: Maximum bytes of @dst__ign, includes the trailing NUL.
> + * @src__ign: Source string.
> + *
> + * 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_strcat(char *dst, u32 dst__sz, const char *src__ign)
> +{
> +	return __bpf_strncat(dst, dst__sz, src__ign, XATTR_SIZE_MAX);
> +}
> +
> +/**
> + * bpf_strncat - Append non-null bytes from a source string, and null-terminate
> + *               the result
> + * @dst: 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, u32 dst__sz, const char *src__ign,
> +			    u32 len)
> +{
> +	return __bpf_strncat(dst, dst__sz, src__ign, len);
> +}
> +
>  #ifdef CONFIG_KEYS
>  /**
>   * bpf_lookup_user_key - lookup a key by its serial
> @@ -4958,6 +5049,8 @@ BTF_ID_FLAGS(func, bpf_strstr);
>  BTF_ID_FLAGS(func, bpf_strcasestr);
>  BTF_ID_FLAGS(func, bpf_strnstr);
>  BTF_ID_FLAGS(func, bpf_strncasestr);
> +BTF_ID_FLAGS(func, bpf_strcat);
> +BTF_ID_FLAGS(func, bpf_strncat);
>  #if defined(CONFIG_BPF_LSM) && defined(CONFIG_CGROUPS)
>  BTF_ID_FLAGS(func, bpf_cgroup_read_xattr, KF_RCU)
>  #endif