Hi,
On 10/18/2023 4:31 AM, Song Liu 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.
>
>> 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?
I think bpf_dynptr_is_rdonly() is also needed. Because the content of
dynptr may be modified by other bpf program and the zero-terminated
condition will not true. As suggested by Alexei, add string support in
verifier is a better choice.
>
> 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));
>>> +}
