[bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Jordan Rome]

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.

Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index d02ae323996b..e87d5df658cb 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2939,6 +2939,41 @@ __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.
+ *
+ * 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.
+ */
+__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
+{
+	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 (ret == count)
+			((char *)dst)[ret] = '\0';
+		ret++;
+	}
+
+	return ret;
+}
+
 __bpf_kfunc_end_defs();

 BTF_KFUNCS_START(generic_btf_ids)
@@ -3024,6 +3059,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 = {
--
2.43.5

[bpf-next v3 2/2] bpf: Add tests for bpf_copy_from_user_str kfunc

[Jordan Rome]

This adds tests for both the happy path and
the error path.

Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 .../selftests/bpf/prog_tests/attach_probe.c   |  8 +++--
 .../selftests/bpf/prog_tests/read_vsyscall.c  |  1 +
 .../selftests/bpf/progs/read_vsyscall.c       |  9 ++++-
 .../selftests/bpf/progs/test_attach_probe.c   | 33 +++++++++++++++++--
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index 7175af39134f..329c7862b52d 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -283,9 +283,11 @@ static void test_uprobe_sleepable(struct test_attach_probe *skel)
 	trigger_func3();

 	ASSERT_EQ(skel->bss->uprobe_byname3_sleepable_res, 9, "check_uprobe_byname3_sleepable_res");
-	ASSERT_EQ(skel->bss->uprobe_byname3_res, 10, "check_uprobe_byname3_res");
-	ASSERT_EQ(skel->bss->uretprobe_byname3_sleepable_res, 11, "check_uretprobe_byname3_sleepable_res");
-	ASSERT_EQ(skel->bss->uretprobe_byname3_res, 12, "check_uretprobe_byname3_res");
+	ASSERT_EQ(skel->bss->uprobe_byname3_str_sleepable_res, 10, "check_uprobe_byname3_str_sleepable_res");
+	ASSERT_EQ(skel->bss->uprobe_byname3_res, 11, "check_uprobe_byname3_res");
+	ASSERT_EQ(skel->bss->uretprobe_byname3_sleepable_res, 12, "check_uretprobe_byname3_sleepable_res");
+	ASSERT_EQ(skel->bss->uretprobe_byname3_str_sleepable_res, 13, "check_uretprobe_byname3_str_sleepable_res");
+	ASSERT_EQ(skel->bss->uretprobe_byname3_res, 14, "check_uretprobe_byname3_res");
 }

 void test_attach_probe(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
index 3405923fe4e6..c7b9ba8b1d06 100644
--- a/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
@@ -23,6 +23,7 @@ struct read_ret_desc {
 	{ .name = "probe_read_user_str", .ret = -EFAULT },
 	{ .name = "copy_from_user", .ret = -EFAULT },
 	{ .name = "copy_from_user_task", .ret = -EFAULT },
+	{ .name = "copy_from_user_str", .ret = -EFAULT },
 };

 void test_read_vsyscall(void)
diff --git a/tools/testing/selftests/bpf/progs/read_vsyscall.c b/tools/testing/selftests/bpf/progs/read_vsyscall.c
index 986f96687ae1..27de1e907754 100644
--- a/tools/testing/selftests/bpf/progs/read_vsyscall.c
+++ b/tools/testing/selftests/bpf/progs/read_vsyscall.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (C) 2024. Huawei Technologies Co., Ltd */
+#include "vmlinux.h"
 #include <linux/types.h>
 #include <bpf/bpf_helpers.h>

@@ -7,10 +8,15 @@

 int target_pid = 0;
 void *user_ptr = 0;
-int read_ret[8];
+int read_ret[9];

 char _license[] SEC("license") = "GPL";

+/*
+ * This is the only kfunc, the others are helpers
+ */
+int bpf_copy_from_user_str(void *dst, u32, const void *) __weak __ksym;
+
 SEC("fentry/" SYS_PREFIX "sys_nanosleep")
 int do_probe_read(void *ctx)
 {
@@ -40,6 +46,7 @@ int do_copy_from_user(void *ctx)
 	read_ret[6] = bpf_copy_from_user(buf, sizeof(buf), user_ptr);
 	read_ret[7] = bpf_copy_from_user_task(buf, sizeof(buf), user_ptr,
 					      bpf_get_current_task_btf(), 0);
+	read_ret[8] = bpf_copy_from_user_str((char *)buf, sizeof(buf), user_ptr);

 	return 0;
 }
diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
index 68466a6ad18c..bf59a5280776 100644
--- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
+++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
@@ -14,11 +14,15 @@ int uretprobe_byname_res = 0;
 int uprobe_byname2_res = 0;
 int uretprobe_byname2_res = 0;
 int uprobe_byname3_sleepable_res = 0;
+int uprobe_byname3_str_sleepable_res = 0;
 int uprobe_byname3_res = 0;
 int uretprobe_byname3_sleepable_res = 0;
+int uretprobe_byname3_str_sleepable_res = 0;
 int uretprobe_byname3_res = 0;
 void *user_ptr = 0;

+int bpf_copy_from_user_str(void *dst, u32, const void *) __weak __ksym;
+
 SEC("ksyscall/nanosleep")
 int BPF_KSYSCALL(handle_kprobe_auto, struct __kernel_timespec *req, struct __kernel_timespec *rem)
 {
@@ -87,11 +91,32 @@ static __always_inline bool verify_sleepable_user_copy(void)
 	return bpf_strncmp(data, sizeof(data), "test_data") == 0;
 }

+static __always_inline bool verify_sleepable_user_copy_str(void)
+{
+	int ret;
+	char data_long[20];
+	char data_short[4];
+
+	ret = bpf_copy_from_user_str(data_short, sizeof(data_short), user_ptr);
+
+	if (bpf_strncmp(data_short, 4, "tes\0") != 0 || ret != 4)
+		return false;
+
+	ret = bpf_copy_from_user_str(data_long, sizeof(data_long), user_ptr);
+
+	if (bpf_strncmp(data_long, 10, "test_data\0") != 0 || ret != 10)
+		return false;
+
+	return true;
+}
+
 SEC("uprobe.s//proc/self/exe:trigger_func3")
 int handle_uprobe_byname3_sleepable(struct pt_regs *ctx)
 {
 	if (verify_sleepable_user_copy())
 		uprobe_byname3_sleepable_res = 9;
+	if (verify_sleepable_user_copy_str())
+		uprobe_byname3_str_sleepable_res = 10;
 	return 0;
 }

@@ -102,7 +127,7 @@ int handle_uprobe_byname3_sleepable(struct pt_regs *ctx)
 SEC("uprobe//proc/self/exe:trigger_func3")
 int handle_uprobe_byname3(struct pt_regs *ctx)
 {
-	uprobe_byname3_res = 10;
+	uprobe_byname3_res = 11;
 	return 0;
 }

@@ -110,14 +135,16 @@ SEC("uretprobe.s//proc/self/exe:trigger_func3")
 int handle_uretprobe_byname3_sleepable(struct pt_regs *ctx)
 {
 	if (verify_sleepable_user_copy())
-		uretprobe_byname3_sleepable_res = 11;
+		uretprobe_byname3_sleepable_res = 12;
+	if (verify_sleepable_user_copy_str())
+		uretprobe_byname3_str_sleepable_res = 13;
 	return 0;
 }

 SEC("uretprobe//proc/self/exe:trigger_func3")
 int handle_uretprobe_byname3(struct pt_regs *ctx)
 {
-	uretprobe_byname3_res = 12;
+	uretprobe_byname3_res = 14;
 	return 0;
 }

--
2.43.5

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Alexei Starovoitov]

On Mon, Aug 12, 2024 at 6:26 PM 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.
>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index d02ae323996b..e87d5df658cb 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2939,6 +2939,41 @@ __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.
> + *
> + * 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.
> + */
> +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> +{
> +       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 (ret == count)
> +                       ((char *)dst)[ret] = '\0';
> +               ret++;
> +       }
> +
> +       return ret;
> +}

The above will not pad the buffer and it will create instability
when the target buffer is a part of the map key. Consider:

struct map_key {
   char str[100];
};
struct {
        __uint(type, BPF_MAP_TYPE_HASH);
        __type(key, struct map_key);
} hash SEC(".maps");

struct map_key key;
bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);

The verifier will think that all of the 'key' is initialized,
but for short strings the key will have garbage.

bpf_probe_read_kernel_str() has the same issue as above, but
let's fix it here first and update read_kernel_str() later.

pw-bot: cr

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Jordan Rome]

On Mon, Aug 12, 2024 at 10:10 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Aug 12, 2024 at 6:26 PM 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.
> >
> > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > ---
> >  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index d02ae323996b..e87d5df658cb 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -2939,6 +2939,41 @@ __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.
> > + *
> > + * 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.
> > + */
> > +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> > +{
> > +       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 (ret == count)
> > +                       ((char *)dst)[ret] = '\0';
> > +               ret++;
> > +       }
> > +
> > +       return ret;
> > +}
>
> The above will not pad the buffer and it will create instability
> when the target buffer is a part of the map key. Consider:
>
> struct map_key {
>    char str[100];
> };
> struct {
>         __uint(type, BPF_MAP_TYPE_HASH);
>         __type(key, struct map_key);
> } hash SEC(".maps");
>
> struct map_key key;
> bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);
>
> The verifier will think that all of the 'key' is initialized,
> but for short strings the key will have garbage.
>
> bpf_probe_read_kernel_str() has the same issue as above, but
> let's fix it here first and update read_kernel_str() later.
>
> pw-bot: cr

You're saying we should always do a memset using `dst__szk` on success
of copying the string?

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Jordan Rome]

On Tue, Aug 13, 2024 at 6:27 AM Jordan Rome <linux@jordanrome.com> wrote:
>
> On Mon, Aug 12, 2024 at 10:10 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Aug 12, 2024 at 6:26 PM 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.
> > >
> > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > ---
> > >  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index d02ae323996b..e87d5df658cb 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -2939,6 +2939,41 @@ __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.
> > > + *
> > > + * 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.
> > > + */
> > > +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> > > +{
> > > +       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 (ret == count)
> > > +                       ((char *)dst)[ret] = '\0';
> > > +               ret++;
> > > +       }
> > > +
> > > +       return ret;
> > > +}
> >
> > The above will not pad the buffer and it will create instability
> > when the target buffer is a part of the map key. Consider:
> >
> > struct map_key {
> >    char str[100];
> > };
> > struct {
> >         __uint(type, BPF_MAP_TYPE_HASH);
> >         __type(key, struct map_key);
> > } hash SEC(".maps");
> >
> > struct map_key key;
> > bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);
> >
> > The verifier will think that all of the 'key' is initialized,
> > but for short strings the key will have garbage.
> >
> > bpf_probe_read_kernel_str() has the same issue as above, but
> > let's fix it here first and update read_kernel_str() later.
> >
> > pw-bot: cr
>
> You're saying we should always do a memset using `dst__szk` on success
> of copying the string?

Something like this?
```
ret = strncpy_from_user(dst, unsafe_ptr__ign, count);
  if (ret >= 0) {
    if (ret <= count)
       memset((char *)dst + ret, 0, dst__szk - ret);
    ret++;
}
```

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Alexei Starovoitov]

On Tue, Aug 13, 2024 at 6:30 AM Jordan Rome <linux@jordanrome.com> wrote:
>
> On Tue, Aug 13, 2024 at 6:27 AM Jordan Rome <linux@jordanrome.com> wrote:
> >
> > On Mon, Aug 12, 2024 at 10:10 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Aug 12, 2024 at 6:26 PM 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.
> > > >
> > > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > > ---
> > > >  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > >
> > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > index d02ae323996b..e87d5df658cb 100644
> > > > --- a/kernel/bpf/helpers.c
> > > > +++ b/kernel/bpf/helpers.c
> > > > @@ -2939,6 +2939,41 @@ __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.
> > > > + *
> > > > + * 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.
> > > > + */
> > > > +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> > > > +{
> > > > +       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 (ret == count)
> > > > +                       ((char *)dst)[ret] = '\0';
> > > > +               ret++;
> > > > +       }
> > > > +
> > > > +       return ret;
> > > > +}
> > >
> > > The above will not pad the buffer and it will create instability
> > > when the target buffer is a part of the map key. Consider:
> > >
> > > struct map_key {
> > >    char str[100];
> > > };
> > > struct {
> > >         __uint(type, BPF_MAP_TYPE_HASH);
> > >         __type(key, struct map_key);
> > > } hash SEC(".maps");
> > >
> > > struct map_key key;
> > > bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);
> > >
> > > The verifier will think that all of the 'key' is initialized,
> > > but for short strings the key will have garbage.
> > >
> > > bpf_probe_read_kernel_str() has the same issue as above, but
> > > let's fix it here first and update read_kernel_str() later.
> > >
> > > pw-bot: cr
> >
> > You're saying we should always do a memset using `dst__szk` on success
> > of copying the string?
>
> Something like this?
> ```
> ret = strncpy_from_user(dst, unsafe_ptr__ign, count);
>   if (ret >= 0) {
>     if (ret <= count)
>        memset((char *)dst + ret, 0, dst__szk - ret);
>     ret++;
> }
> ```

yep. something like this. I didn't check the math.

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Andrii Nakryiko]

On Tue, Aug 13, 2024 at 9:08 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Aug 13, 2024 at 6:30 AM Jordan Rome <linux@jordanrome.com> wrote:
> >
> > On Tue, Aug 13, 2024 at 6:27 AM Jordan Rome <linux@jordanrome.com> wrote:
> > >
> > > On Mon, Aug 12, 2024 at 10:10 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 12, 2024 at 6:26 PM 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.
> > > > >
> > > > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > > > ---
> > > > >  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 36 insertions(+)
> > > > >
> > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > > index d02ae323996b..e87d5df658cb 100644
> > > > > --- a/kernel/bpf/helpers.c
> > > > > +++ b/kernel/bpf/helpers.c
> > > > > @@ -2939,6 +2939,41 @@ __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.
> > > > > + *
> > > > > + * 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.
> > > > > + */
> > > > > +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> > > > > +{
> > > > > +       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 (ret == count)
> > > > > +                       ((char *)dst)[ret] = '\0';
> > > > > +               ret++;
> > > > > +       }
> > > > > +
> > > > > +       return ret;
> > > > > +}
> > > >
> > > > The above will not pad the buffer and it will create instability
> > > > when the target buffer is a part of the map key. Consider:
> > > >
> > > > struct map_key {
> > > >    char str[100];
> > > > };
> > > > struct {
> > > >         __uint(type, BPF_MAP_TYPE_HASH);
> > > >         __type(key, struct map_key);
> > > > } hash SEC(".maps");
> > > >
> > > > struct map_key key;
> > > > bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);
> > > >
> > > > The verifier will think that all of the 'key' is initialized,
> > > > but for short strings the key will have garbage.
> > > >
> > > > bpf_probe_read_kernel_str() has the same issue as above, but
> > > > let's fix it here first and update read_kernel_str() later.
> > > >
> > > > pw-bot: cr
> > >
> > > You're saying we should always do a memset using `dst__szk` on success
> > > of copying the string?
> >
> > Something like this?
> > ```
> > ret = strncpy_from_user(dst, unsafe_ptr__ign, count);
> >   if (ret >= 0) {
> >     if (ret <= count)
> >        memset((char *)dst + ret, 0, dst__szk - ret);
> >     ret++;
> > }
> > ```
>
> yep. something like this. I didn't check the math.

I'm a bit worried about this unconditional memset without having a way
to disable it. In practice, lots of cases won't use the destination
buffer as a map key, but rather just send it over ringbuf. So paying
the price of zeroing out seems unnecessary.

It's quite often (I do that in retsnoop, for instance; and we have
other cases in our production) that we have a pretty big buffer, but
expect that most of the time strings will be much smaller. So we can
have a 1K buffer, but get 20 bytes of string content (and we end up
sending only actual useful size of data over ringbuf/perfbuf, so not
even paying 1K memcpy() overhead). Paying for memset()'ing the entire
1K (and string reading can happen in a loop, so this memsetting will
be happening over and over, unnecessarily), seems excessive.

Given it's pretty easy to do memset(0) using bpf_prober_read(dst, sz,
NULL), maybe we shouldn't do memsetting unconditionally? We can add a
loud comment stating the danger of using the resulting buffer as map
key without clearing the unfilled part of the buffer and that should
be sufficient?

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Alexei Starovoitov]

On Tue, Aug 13, 2024 at 11:10 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Aug 13, 2024 at 9:08 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Aug 13, 2024 at 6:30 AM Jordan Rome <linux@jordanrome.com> wrote:
> > >
> > > On Tue, Aug 13, 2024 at 6:27 AM Jordan Rome <linux@jordanrome.com> wrote:
> > > >
> > > > On Mon, Aug 12, 2024 at 10:10 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Aug 12, 2024 at 6:26 PM 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.
> > > > > >
> > > > > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > > > > ---
> > > > > >  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 36 insertions(+)
> > > > > >
> > > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > > > index d02ae323996b..e87d5df658cb 100644
> > > > > > --- a/kernel/bpf/helpers.c
> > > > > > +++ b/kernel/bpf/helpers.c
> > > > > > @@ -2939,6 +2939,41 @@ __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.
> > > > > > + *
> > > > > > + * 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.
> > > > > > + */
> > > > > > +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> > > > > > +{
> > > > > > +       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 (ret == count)
> > > > > > +                       ((char *)dst)[ret] = '\0';
> > > > > > +               ret++;
> > > > > > +       }
> > > > > > +
> > > > > > +       return ret;
> > > > > > +}
> > > > >
> > > > > The above will not pad the buffer and it will create instability
> > > > > when the target buffer is a part of the map key. Consider:
> > > > >
> > > > > struct map_key {
> > > > >    char str[100];
> > > > > };
> > > > > struct {
> > > > >         __uint(type, BPF_MAP_TYPE_HASH);
> > > > >         __type(key, struct map_key);
> > > > > } hash SEC(".maps");
> > > > >
> > > > > struct map_key key;
> > > > > bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);
> > > > >
> > > > > The verifier will think that all of the 'key' is initialized,
> > > > > but for short strings the key will have garbage.
> > > > >
> > > > > bpf_probe_read_kernel_str() has the same issue as above, but
> > > > > let's fix it here first and update read_kernel_str() later.
> > > > >
> > > > > pw-bot: cr
> > > >
> > > > You're saying we should always do a memset using `dst__szk` on success
> > > > of copying the string?
> > >
> > > Something like this?
> > > ```
> > > ret = strncpy_from_user(dst, unsafe_ptr__ign, count);
> > >   if (ret >= 0) {
> > >     if (ret <= count)
> > >        memset((char *)dst + ret, 0, dst__szk - ret);
> > >     ret++;
> > > }
> > > ```
> >
> > yep. something like this. I didn't check the math.
>
> I'm a bit worried about this unconditional memset without having a way
> to disable it. In practice, lots of cases won't use the destination
> buffer as a map key, but rather just send it over ringbuf. So paying
> the price of zeroing out seems unnecessary.
>
> It's quite often (I do that in retsnoop, for instance; and we have
> other cases in our production) that we have a pretty big buffer, but
> expect that most of the time strings will be much smaller. So we can
> have a 1K buffer, but get 20 bytes of string content (and we end up
> sending only actual useful size of data over ringbuf/perfbuf, so not
> even paying 1K memcpy() overhead). Paying for memset()'ing the entire
> 1K (and string reading can happen in a loop, so this memsetting will
> be happening over and over, unnecessarily), seems excessive.
>
> Given it's pretty easy to do memset(0) using bpf_prober_read(dst, sz,
> NULL), maybe we shouldn't do memsetting unconditionally? We can add a
> loud comment stating the danger of using the resulting buffer as map
> key without clearing the unfilled part of the buffer and that should
> be sufficient?

probe_read as memset is a quirk that folks learned to abuse.
Let's add a flag to this bpf_copy_from_user_str() kfunc instead,
so it behaves either like strscpy_pad or strscpy.

Re: [bpf-next v3 1/2] bpf: Add bpf_copy_from_user_str kfunc

[Andrii Nakryiko]

On Tue, Aug 13, 2024 at 11:30 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Aug 13, 2024 at 11:10 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Aug 13, 2024 at 9:08 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Aug 13, 2024 at 6:30 AM Jordan Rome <linux@jordanrome.com> wrote:
> > > >
> > > > On Tue, Aug 13, 2024 at 6:27 AM Jordan Rome <linux@jordanrome.com> wrote:
> > > > >
> > > > > On Mon, Aug 12, 2024 at 10:10 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Aug 12, 2024 at 6:26 PM 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.
> > > > > > >
> > > > > > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > > > > > ---
> > > > > > >  kernel/bpf/helpers.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 36 insertions(+)
> > > > > > >
> > > > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > > > > index d02ae323996b..e87d5df658cb 100644
> > > > > > > --- a/kernel/bpf/helpers.c
> > > > > > > +++ b/kernel/bpf/helpers.c
> > > > > > > @@ -2939,6 +2939,41 @@ __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.
> > > > > > > + *
> > > > > > > + * 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.
> > > > > > > + */
> > > > > > > +__bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__szk, const void __user *unsafe_ptr__ign)
> > > > > > > +{
> > > > > > > +       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 (ret == count)
> > > > > > > +                       ((char *)dst)[ret] = '\0';
> > > > > > > +               ret++;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       return ret;
> > > > > > > +}
> > > > > >
> > > > > > The above will not pad the buffer and it will create instability
> > > > > > when the target buffer is a part of the map key. Consider:
> > > > > >
> > > > > > struct map_key {
> > > > > >    char str[100];
> > > > > > };
> > > > > > struct {
> > > > > >         __uint(type, BPF_MAP_TYPE_HASH);
> > > > > >         __type(key, struct map_key);
> > > > > > } hash SEC(".maps");
> > > > > >
> > > > > > struct map_key key;
> > > > > > bpf_copy_from_user_str(key.str, sizeof(key.str), user_string);
> > > > > >
> > > > > > The verifier will think that all of the 'key' is initialized,
> > > > > > but for short strings the key will have garbage.
> > > > > >
> > > > > > bpf_probe_read_kernel_str() has the same issue as above, but
> > > > > > let's fix it here first and update read_kernel_str() later.
> > > > > >
> > > > > > pw-bot: cr
> > > > >
> > > > > You're saying we should always do a memset using `dst__szk` on success
> > > > > of copying the string?
> > > >
> > > > Something like this?
> > > > ```
> > > > ret = strncpy_from_user(dst, unsafe_ptr__ign, count);
> > > >   if (ret >= 0) {
> > > >     if (ret <= count)
> > > >        memset((char *)dst + ret, 0, dst__szk - ret);
> > > >     ret++;
> > > > }
> > > > ```
> > >
> > > yep. something like this. I didn't check the math.
> >
> > I'm a bit worried about this unconditional memset without having a way
> > to disable it. In practice, lots of cases won't use the destination
> > buffer as a map key, but rather just send it over ringbuf. So paying
> > the price of zeroing out seems unnecessary.
> >
> > It's quite often (I do that in retsnoop, for instance; and we have
> > other cases in our production) that we have a pretty big buffer, but
> > expect that most of the time strings will be much smaller. So we can
> > have a 1K buffer, but get 20 bytes of string content (and we end up
> > sending only actual useful size of data over ringbuf/perfbuf, so not
> > even paying 1K memcpy() overhead). Paying for memset()'ing the entire
> > 1K (and string reading can happen in a loop, so this memsetting will
> > be happening over and over, unnecessarily), seems excessive.
> >
> > Given it's pretty easy to do memset(0) using bpf_prober_read(dst, sz,
> > NULL), maybe we shouldn't do memsetting unconditionally? We can add a
> > loud comment stating the danger of using the resulting buffer as map
> > key without clearing the unfilled part of the buffer and that should
> > be sufficient?
>
> probe_read as memset is a quirk that folks learned to abuse.
> Let's add a flag to this bpf_copy_from_user_str() kfunc instead,
> so it behaves either like strscpy_pad or strscpy.

agreed, a flag sounds good