On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@xxxxxxxx> wrote:
>
>
>
> > On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@xxxxxxxxxx> wrote:
> >>
> >> This kfunc can be used to read xattr of a file.
> >>
> >> Since vfs_getxattr() requires null-terminated string as input "name", a new
> >> helper bpf_dynptr_is_string() is added to check the input before calling
> >> vfs_getxattr().
> >>
> >> Signed-off-by: Song Liu <song@xxxxxxxxxx>
> >> ---
> >> include/linux/bpf.h | 12 +++++++++++
> >> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 56 insertions(+)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index 61bde4520f5c..f14fae45e13d 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
> >> return !!current->bpf_ctx;
> >> }
> >>
> >> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> >
> > is_zero_terminated would be more accurate? though there is nothing
> > really dynptr-specific here...
>
> is_zero_terminated sounds better.
>
> >
> >> +{
> >> + char *str = ptr->data;
> >> +
> >> + return str[__bpf_dynptr_size(ptr) - 1] == '\0';
> >> +}
> >> +
> >> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
> >>
> >> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
> >> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
> >> return false;
> >> }
> >>
> >> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> >> +{
> >> + return false;
> >> +}
> >> +
> >> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
> >> {
> >> }
> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >> index df697c74d519..946268574e05 100644
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -24,6 +24,7 @@
> >> #include <linux/key.h>
> >> #include <linux/verification.h>
> >> #include <linux/namei.h>
> >> +#include <linux/fileattr.h>
> >>
> >> #include <net/bpf_sk_storage.h>
> >>
> >> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
> >> late_initcall(bpf_key_sig_kfuncs_init);
> >> #endif /* CONFIG_KEYS */
> >>
> >> +/* filesystem kfuncs */
> >> +__diag_push();
> >> +__diag_ignore_all("-Wmissing-prototypes",
> >> + "kfuncs which will be used in BPF programs");
> >> +
> >> +/**
> >> + * bpf_get_file_xattr - get xattr of a file
> >> + * @name_ptr: name of the xattr
> >> + * @value_ptr: output buffer of the xattr value
> >> + *
> >> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
> >> + *
> >> + * Return: 0 on success, a negative value on error.
> >> + */
> >> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> >> + struct bpf_dynptr_kern *value_ptr)
> >> +{
> >> + if (!bpf_dynptr_is_string(name_ptr))
> >> + return -EINVAL;
> >
> > so dynptr can be invalid and name_ptr->data will be NULL, you should
> > account for that
>
> We can add a NULL check (or size check) here.
there must be some helper to check if dynptr is valid, let's use that
instead of NULL checks
>
> >
> > and there could also be special dynptrs that don't have contiguous
> > memory region, so somehow you'd need to take care of that as well
>
> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
> we need this for dynptr of skb or xdp. Would this be sufficient?
well, to keep thing simple we can have a simple internal helper API
that will tell if it's safe to assume that dynptr memory is contiguous
and it's ok to use dynptr memory. But still, you shouldn't access data
pointer directly, there must be some helper for that. Please check. It
has to take into account offset and stuff like that.
Also, and separately from that, we should think about providing a
bpf_dynptr_slice()-like helper that will accept a fixed-sized
temporary buffer and return pointer to either actual memory or copy
non-contiguous memory into that buffer. That will make sure you can
use any dynptr as a source of data, and only pay the price of memory
copy in rare cases where it's necessary
>
> Thanks,
> Song
>
> >
> >> +
> >> + return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
> >> + value_ptr->data, __bpf_dynptr_size(value_ptr));
> >> +}
>
