| Message ID | 20231013182644.2346458-2-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: file verification with LSM and fsverity | expand |
On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> > --- > 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... > +{ > + 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 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 > + > + return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data, > + value_ptr->data, __bpf_dynptr_size(value_ptr)); > +} > + > +__diag_pop(); > + > +BTF_SET8_START(fs_kfunc_set) > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE) > +BTF_SET8_END(fs_kfunc_set) > + > +const struct btf_kfunc_id_set bpf_fs_kfunc_set = { > + .owner = THIS_MODULE, > + .set = &fs_kfunc_set, > +}; > + > +static int __init bpf_fs_kfuncs_init(void) > +{ > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, > + &bpf_fs_kfunc_set); > +} > + > +late_initcall(bpf_fs_kfuncs_init); > + > static const struct bpf_func_proto * > bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > { > -- > 2.34.1 >
On Fri, Oct 13, 2023 at 11:30 AM Song Liu <song@kernel.org> wrote: > +__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; > + > + return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data, > + value_ptr->data, __bpf_dynptr_size(value_ptr)); > +} > + > +__diag_pop(); > + > +BTF_SET8_START(fs_kfunc_set) > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE) I suspect it needs to be allowlisted too. Sleepable might not be enough. KP proposed such kfunc in the past and there were recursion issues. KP, do you remember the details?
> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> >> --- >> 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? 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)); >> +}
On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote: > > > > > On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> > >> --- > >> 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)); > >> +} >
> On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote: >> >> >> >>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>> >>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> >>>> --- >>>> 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 Yeah, we can use bpf_dynptr_is_null(). > >> >>> >>> 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. Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc). > > 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 I don't quite follow here. Currently, we have bpf_dynptr_data() bpf_dynptr_slice() bpf_dynptr_slice_rdwr() bpf_dynptr_write() AFAICT, they are sufficient to cover existing use cases (and the new use case we are adding in this set). What's the new kfunc are you thinking about? Thanks, Song
On Tue, Oct 17, 2023 at 3:16 PM Song Liu <songliubraving@meta.com> wrote: > > > > > On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote: > >> > >> > >> > >>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > >>> > >>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> > >>>> --- > >>>> 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 > > Yeah, we can use bpf_dynptr_is_null(). > > > > >> > >>> > >>> 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. > > Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc). > > > > > 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 > > I don't quite follow here. Currently, we have > > bpf_dynptr_data() > bpf_dynptr_slice() > bpf_dynptr_slice_rdwr() > bpf_dynptr_write() > > AFAICT, they are sufficient to cover existing use cases (and the new > use case we are adding in this set). What's the new kfunc are you > thinking about? I wasn't talking about kfuncs, but rather just internal helpers to be used by other kfuncs when working with dynptrs as input arguments. > > Thanks, > Song > >
> On Oct 17, 2023, at 3:40 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 17, 2023 at 3:16 PM Song Liu <songliubraving@meta.com> wrote: >> >> >> >>> On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>> >>> On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote: >>>> >>>> >>>> >>>>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>>>> >>>>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> >>>>>> --- >>>>>> 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 >> >> Yeah, we can use bpf_dynptr_is_null(). >> >>> >>>> >>>>> >>>>> 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. >> >> Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc). >> >>> >>> 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 >> >> I don't quite follow here. Currently, we have >> >> bpf_dynptr_data() >> bpf_dynptr_slice() >> bpf_dynptr_slice_rdwr() >> bpf_dynptr_write() >> >> AFAICT, they are sufficient to cover existing use cases (and the new >> use case we are adding in this set). What's the new kfunc are you >> thinking about? > > I wasn't talking about kfuncs, but rather just internal helpers to be > used by other kfuncs when working with dynptrs as input arguments. AFAICT, kfuncs can call other kfuncs, for example bpf_dynptr_slice_rdwr() calls bpf_dynptr_slice(). This is limited to the same file at the moment, since we do not expose kfuncs in headers. Thanks, Song
Hi, On 10/18/2023 4:31 AM, Song Liu wrote: > >> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >> >> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> 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@kernel.org> >>> --- >>> 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)); >>> +}
On Tue, Oct 17, 2023 at 9:11 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Oct 13, 2023 at 11:30 AM Song Liu <song@kernel.org> wrote: > > +__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; > > + > > + return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data, > > + value_ptr->data, __bpf_dynptr_size(value_ptr)); > > +} > > + > > +__diag_pop(); > > + > > +BTF_SET8_START(fs_kfunc_set) > > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE) > > I suspect it needs to be allowlisted too. > Sleepable might not be enough. > > KP proposed such kfunc in the past and there were recursion issues. > > KP, > do you remember the details? yeah, have a look at Al's reply: https://lore.kernel.org/bpf/Yrs4+ThR4ACb5eD%2F@ZenIV/ it can create deadlocks and potentially UAFs (similar to the situation Jann mentioned). This will need to be allowlisted.
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) +{ + 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; + + return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data, + value_ptr->data, __bpf_dynptr_size(value_ptr)); +} + +__diag_pop(); + +BTF_SET8_START(fs_kfunc_set) +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE) +BTF_SET8_END(fs_kfunc_set) + +const struct btf_kfunc_id_set bpf_fs_kfunc_set = { + .owner = THIS_MODULE, + .set = &fs_kfunc_set, +}; + +static int __init bpf_fs_kfuncs_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, + &bpf_fs_kfunc_set); +} + +late_initcall(bpf_fs_kfuncs_init); + static const struct bpf_func_proto * bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) {
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@kernel.org> --- include/linux/bpf.h | 12 +++++++++++ kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)