Re: [bpf-next v3 2/3] bpf: Add bpf_copy_from_user_task_str kfunc

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jan 24, 2025 at 10:33 AM Jordan Rome <linux@jordanrome.com> wrote:
>
> This new kfunc will be able to copy a string
> from another process's/task's address space.
> This is similar to `bpf_copy_from_user_str`
> but accepts a `struct task_struct*` argument.
>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  kernel/bpf/helpers.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index f27ce162427a..c26fabf97afd 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -3082,6 +3082,56 @@ __bpf_kfunc void bpf_local_irq_restore(unsigned long *flags__irq_flag)
>         local_irq_restore(*flags__irq_flag);
>  }
>
> +/**
> + * bpf_copy_from_user_task_str() - Copy a string from an task's address space
> + * @dst:             Destination address, in kernel space.  This buffer must be
> + *                   at least @dst__sz bytes long.
> + * @dst__sz:         Maximum number of bytes to copy, includes the trailing NUL.
> + * @unsafe_ptr__ign: Source address in the task's address space.
> + * @tsk:             The task whose address space will be used
> + * @flags:           The only supported flag is BPF_F_PAD_ZEROS
> + *
> + * Copies a NULL-terminated string from a task's address space to *dst* buffer.
> + * If user string is too long this will still ensure zero termination in the
> + * dst buffer unless buffer size is 0.
> + *
> + * If BPF_F_PAD_ZEROS flag is set, memset the tail of @dst to 0 on success and
> + * memset all of @dst on failure.
> + *
> + * Return: The number of copied bytes on success, including the NULL-terminator.

s/NULL/NUL/

Other than that, LGTM:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> + * A negative error code on failure.
> + */
> +__bpf_kfunc int bpf_copy_from_user_task_str(void *dst,
> +                                           u32 dst__sz,
> +                                           const void __user *unsafe_ptr__ign,
> +                                           struct task_struct *tsk,
> +                                           u64 flags)
> +{
> +       int ret = 0;

nit: no need to zero initialize, you'll overwrite it unconditionally below
> +
> +       if (unlikely(flags & ~BPF_F_PAD_ZEROS))
> +               return -EINVAL;
> +
> +       if (unlikely(!dst__sz))
> +               return 0;
> +
> +       ret = copy_remote_vm_str(tsk, (unsigned long)unsafe_ptr__ign, dst, dst__sz, 0);
> +
> +       if (ret <= 0) {
> +               if (flags & BPF_F_PAD_ZEROS)
> +                       memset(dst, 0, dst__sz);
> +               return ret ?: -EINVAL;
> +       }
> +
> +       if (ret < dst__sz) {
> +               if (flags & BPF_F_PAD_ZEROS)
> +                       memset(dst + ret, 0, dst__sz - ret);
> +               return ret + 1;
> +       }
> +
> +       return ret;
> +}
> +
>  __bpf_kfunc_end_defs();
>
>  BTF_KFUNCS_START(generic_btf_ids)
> @@ -3174,6 +3224,7 @@ 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_ID_FLAGS(func, bpf_copy_from_user_task_str, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_get_kmem_cache)
>  BTF_ID_FLAGS(func, bpf_iter_kmem_cache_new, KF_ITER_NEW | KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLEEPABLE)
> --
> 2.43.5
>