On 7/16/26 11:34, Rong Tao wrote: > From: Rong Tao <[email protected]> > > 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 <[email protected]> > --- > 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
