Re: [bpf-next v5 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Kui-Feng Lee <sinquersw@gmail.com>]

On 8/15/24 15:38, Andrii Nakryiko wrote:
> On Thu, Aug 15, 2024 at 4:28 AM Jordan Rome <linux@jordanrome.com> 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@jordanrome.com>
>> ---
>>   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
>>