| Message ID | 20240815112733.4100387-1-linux@jordanrome.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v5,1/2] bpf: Add bpf_copy_from_user_str kfunc | expand |
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? 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 >
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 >>
On Fri, Aug 16, 2024 at 12:23 AM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > > > 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. > ah, I forgot that count is adjusted size already, ok > > > > 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 > >>
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) +}; + #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; + + if (unlikely(!dst__szk)) + return 0; + + count = dst__szk - 1; + if (unlikely(!count)) { + ((char *)dst)[0] = '\0'; + return 1; + } + + 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++; + } + + 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__ */
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(+) -- 2.43.5