On Fri, Aug 16, 2024 at 12:23 AM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> 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.
>
ah, I forgot that count is adjusted size already, ok
> >
> > 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
> >>
