On Thu, Aug 15, 2024 at 4:28 AM Jordan Rome <linux@xxxxxxxxxxxxxx> wrote:
>
> This adds a kfunc wrapper around strncpy_from_user,
> which can be called from sleepable BPF programs.
>
> This matches the non-sleepable 'bpf_probe_read_user_str'
> helper except it includes an additional 'flags'
> param, which allows consumers to clear the entire
> destination buffer on success.
>
> Signed-off-by: Jordan Rome <linux@xxxxxxxxxxxxxx>
> ---
> include/uapi/linux/bpf.h | 8 +++++++
> kernel/bpf/helpers.c | 41 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 8 +++++++
> 3 files changed, 57 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e05b39e39c3f..e207175981be 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7513,4 +7513,12 @@ struct bpf_iter_num {
> __u64 __opaque[1];
> } __attribute__((aligned(8)));
>
> +/*
> + * Flags to control bpf_copy_from_user_str() behaviour.
> + * - BPF_ZERO_BUFFER: Memset 0 the tail of the destination buffer on success
> + */
> +enum {
> + BPF_ZERO_BUFFER = (1ULL << 0)
We call all flags BPF_F_<something>, so let's stay consistent.
And just for a bit of bikeshedding, "zero buffer" isn't immediately
clear and it would be nice to have a clearer verb in there. I don't
have a perfect name, but something like BPF_F_PAD_ZEROS or something
with "pad" maybe?
Also, should we keep behavior a bit more consistent and say that on
failure this flag will also ensure that buffer is cleared?
> +};
> +
> #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index d02ae323996b..fe4348679d38 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2939,6 +2939,46 @@ __bpf_kfunc void bpf_iter_bits_destroy(struct bpf_iter_bits *it)
> bpf_mem_free(&bpf_global_ma, kit->bits);
> }
>
> +/**
> + * bpf_copy_from_user_str() - Copy a string from an unsafe user address
> + * @dst: Destination address, in kernel space. This buffer must be at
> + * least @dst__szk bytes long.
> + * @dst__szk: Maximum number of bytes to copy, including the trailing NUL.
> + * @unsafe_ptr__ign: Source address, in user space.
> + * @flags: The only supported flag is BPF_ZERO_BUFFER
> + *
> + * Copies a NUL-terminated string from userspace to BPF space. If user string is
> + * too long this will still ensure zero termination in the dst buffer unless
> + * buffer size is 0.
> + *
> + * If BPF_ZERO_BUFFER flag is set, memset the tail of @dst to 0 on success.
> + */
> +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign, u64 flags)
> +{
> + int ret;
> + int count;
> +
validate that flags doesn't have any unknown flags
if (unlikely(flags & ~BPF_F_ZERO_BUFFER))
return -EINVAL;
> + if (unlikely(!dst__szk))
> + return 0;
> +
> + count = dst__szk - 1;
> + if (unlikely(!count)) {
> + ((char *)dst)[0] = '\0';
> + return 1;
> + }
Do we need to special-case this unlikely scenario? Especially that
it's unlikely, why write code for it and pay a tiny price for an extra
check?
> +
> + ret = strncpy_from_user(dst, unsafe_ptr__ign, count);
> + if (ret >= 0) {
> + if (flags & BPF_ZERO_BUFFER)
> + memset((char *)dst + ret, 0, dst__szk - ret);
> + else
> + ((char *)dst)[ret] = '\0';
> + ret++;
so if string is truncated, ret == count, no? And dst[ret] will go
beyond the buffer?
we need more tests to validate all those various conditions
I'd also rewrite this a bit, so it's more linear:
ret = strncpy(...);
if (ret < 0)
return ret;
((char *)dst)[count - 1] = '\0';
if (flags & BPF_F_ZERO_BUF)
memset(...);
return ret < count ? ret + 1 : count;
or something along those lines
pw-bot: cr
> + }
> +
> + return ret;
> +}
> +
> __bpf_kfunc_end_defs();
>
> BTF_KFUNCS_START(generic_btf_ids)
> @@ -3024,6 +3064,7 @@ BTF_ID_FLAGS(func, bpf_preempt_enable)
> BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW)
> BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL)
> BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY)
> +BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE)
> BTF_KFUNCS_END(common_btf_ids)
>
> static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index e05b39e39c3f..15c2c3431e0f 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -7513,4 +7513,12 @@ struct bpf_iter_num {
> __u64 __opaque[1];
> } __attribute__((aligned(8)));
>
> +/*
> + * Flags to control bpf_copy_from_user_str() behaviour.
> + * - BPF_ZERO_BUFFER: Memset 0 the entire destination buffer on success
> + */
> +enum {
> + BPF_ZERO_BUFFER = (1ULL << 0)
> +};
> +
> #endif /* _UAPI__LINUX_BPF_H__ */
> --
> 2.43.5
>
