On 8/15/24 15:38, Andrii Nakryiko wrote:
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?
Since count = dst__szk - 1, it is not going 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
