[PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface

[PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface

[Lorenz Bauer]

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.

Fix this by using bpf_attr.test.data_size_out as a size hint. The old
behaviour is retained if size_hint is zero.

Interestingly, do_test_single() in test_verifier.c already assumes
that this is the intended use of data_size_out, and sets it to the
output buffer size.

Lorenz Bauer (3):
  bpf: respect size hint to BPF_PROG_TEST_RUN if present
  libbpf: require size hint in bpf_prog_test_run
  selftests: add a test for bpf_prog_test_run output size

 net/bpf/test_run.c                       |  9 ++++-
 tools/lib/bpf/bpf.c                      |  4 ++-
 tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/3] bpf: respect size hint to BPF_PROG_TEST_RUN if present

[Lorenz Bauer]

Use data_size_out as a size hint when copying test output to user space.
A program using BPF_PERF_OUTPUT can compare its own buffer length with
data_size_out after the syscall to detect whether truncation has taken
place. Callers which so far did not set data_size_in are not affected.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 net/bpf/test_run.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..30c57b7f4ba4 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,15 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 {
 	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
 	int err = -EFAULT;
+	u32 copy_size = size;
 
-	if (data_out && copy_to_user(data_out, data, size))
+	/* Clamp copy if the user has provided a size hint, but copy the full
+	 * buffer if not to retain old behaviour.
+	 */
+	if (kattr->test.data_size_out && copy_size > kattr->test.data_size_out)
+		copy_size = kattr->test.data_size_out;
+
+	if (data_out && copy_to_user(data_out, data, copy_size))
 		goto out;
 	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
 		goto out;
-- 
2.17.1

[PATCH 2/3] libbpf: require size hint in bpf_prog_test_run

[Lorenz Bauer]

Require size_out to be non-NULL if data_out is given. This prevents
accidental overwriting of process memory after the output buffer.

Adjust callers of bpf_prog_test_run to this behaviour.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/lib/bpf/bpf.c                      |  4 +++-
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 03f9bcc4ef50..127a9aa6170e 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -403,10 +403,12 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	attr.test.data_in = ptr_to_u64(data);
 	attr.test.data_out = ptr_to_u64(data_out);
 	attr.test.data_size_in = size;
+	if (data_out)
+		attr.test.data_size_out = *size_out;
 	attr.test.repeat = repeat;
 
 	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
-	if (size_out)
+	if (data_out)
 		*size_out = attr.test.data_size_out;
 	if (retval)
 		*retval = attr.test.retval;
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 2d3c04f45530..560d7527b86b 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -150,6 +150,7 @@ static void test_xdp(void)
 	bpf_map_update_elem(map_fd, &key4, &value4, 0);
 	bpf_map_update_elem(map_fd, &key6, &value6, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 
@@ -158,6 +159,7 @@ static void test_xdp(void)
 	      "err %d errno %d retval %d size %d\n",
 	      err, errno, retval, size);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != XDP_TX || size != 114 ||
@@ -182,6 +184,7 @@ static void test_xdp_adjust_tail(void)
 		return;
 	}
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 
@@ -189,6 +192,7 @@ static void test_xdp_adjust_tail(void)
 	      "ipv4", "err %d errno %d retval %d size %d\n",
 	      err, errno, retval, size);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != XDP_TX || size != 54,
@@ -252,6 +256,7 @@ static void test_l4lb(const char *file)
 		goto out;
 	bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 54 ||
@@ -259,6 +264,7 @@ static void test_l4lb(const char *file)
 	      "err %d errno %d retval %d size %d magic %x\n",
 	      err, errno, retval, size, *magic);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 74 ||
@@ -341,6 +347,7 @@ static void test_xdp_noinline(void)
 		goto out;
 	bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 1 || size != 54 ||
@@ -348,6 +355,7 @@ static void test_xdp_noinline(void)
 	      "err %d errno %d retval %d size %d magic %x\n",
 	      err, errno, retval, size, *magic);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 1 || size != 74 ||
@@ -1795,6 +1803,7 @@ static void test_queue_stack_map(int type)
 			pkt_v4.iph.saddr = vals[MAP_SIZE - 1 - i] * 5;
 		}
 
+		size = sizeof(buf);
 		err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 					buf, &size, &retval, &duration);
 		if (err || retval || size != sizeof(pkt_v4) ||
@@ -1808,6 +1817,7 @@ static void test_queue_stack_map(int type)
 	      err, errno, retval, size, iph->daddr);
 
 	/* Queue is empty, program should return TC_ACT_SHOT */
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 2 /* TC_ACT_SHOT */|| size != sizeof(pkt_v4),
-- 
2.17.1

[PATCH 3/3] selftests: add a test for bpf_prog_test_run output size

[Lorenz Bauer]

Make sure that bpf_prog_test_run returns the correct length
in the size_out argument and that the kernel respects the
output size hint.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 560d7527b86b..6ab98e10e86f 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -124,6 +124,39 @@ static void test_pkt_access(void)
 	bpf_object__close(obj);
 }
 
+static void test_output_size_hint(void)
+{
+	const char *file = "./test_pkt_access.o";
+	struct bpf_object *obj;
+	__u32 retval, size, duration;
+	int err, prog_fd;
+	char buf[10];
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+	if (err) {
+		error_cnt++;
+		return;
+	}
+
+	memset(buf, 0, sizeof(buf));
+
+	size = 5;
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				buf, &size, &retval, &duration);
+	CHECK(err || retval, "run",
+	      "err %d errno %d retval %d\n",
+	      err, errno, retval);
+
+	CHECK(size != sizeof(pkt_v4), "out_size",
+	      "incorrect output size, want %lu have %u\n",
+	      sizeof(pkt_v4), size);
+
+	CHECK(buf[5] != 0, "overflow",
+	      "prog_test_run ignored size hint\n");
+
+	bpf_object__close(obj);
+}
+
 static void test_xdp(void)
 {
 	struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1847,6 +1880,7 @@ int main(void)
 	jit_enabled = is_jit_enabled();
 
 	test_pkt_access();
+	test_output_size_hint();
 	test_xdp();
 	test_xdp_adjust_tail();
 	test_l4lb_all();
-- 
2.17.1

Re: [PATCH 1/3] bpf: respect size hint to BPF_PROG_TEST_RUN if present

[Y Song]

On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Use data_size_out as a size hint when copying test output to user space.
> A program using BPF_PERF_OUTPUT can compare its own buffer length with
> data_size_out after the syscall to detect whether truncation has taken
> place. Callers which so far did not set data_size_in are not affected.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  net/bpf/test_run.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index c89c22c49015..30c57b7f4ba4 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -74,8 +74,15 @@ static int bpf_test_finish(const union bpf_attr *kattr,
>  {
>         void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
>         int err = -EFAULT;
> +       u32 copy_size = size;
>
> -       if (data_out && copy_to_user(data_out, data, size))
> +       /* Clamp copy if the user has provided a size hint, but copy the full
> +        * buffer if not to retain old behaviour.
> +        */
> +       if (kattr->test.data_size_out && copy_size > kattr->test.data_size_out)
> +               copy_size = kattr->test.data_size_out;
> +
> +       if (data_out && copy_to_user(data_out, data, copy_size))
>                 goto out;
>         if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
>                 goto out;

if copy_size < size, maybe we should return -ENOSPC so user space is aware
of insufficient size and takes proper action? This behavior will then
be consistent
with BPF_PROG_QUERY subcommand where prog_cnt is the in/out parameter and
-ENOSPC is returned if kernel does not have enough space to copy out
the whole data.

Also, since data_size_out field now has more complex semantics, could you add
some comments in uapi/linux/bpf.h so it will be relatively clear from
uapi header
how this field will be used?

> --
> 2.17.1
>

Re: [PATCH 2/3] libbpf: require size hint in bpf_prog_test_run

[Y Song]

On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Require size_out to be non-NULL if data_out is given. This prevents
> accidental overwriting of process memory after the output buffer.
>
> Adjust callers of bpf_prog_test_run to this behaviour.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/lib/bpf/bpf.c                      |  4 +++-
>  tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 03f9bcc4ef50..127a9aa6170e 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -403,10 +403,12 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
>         attr.test.data_in = ptr_to_u64(data);
>         attr.test.data_out = ptr_to_u64(data_out);
>         attr.test.data_size_in = size;
> +       if (data_out)
> +               attr.test.data_size_out = *size_out;

Maybe it is better to return error (-EINVAL) instead of segfault if
size_out is NULL?
we should try to avoid segfault inside the library. This will change
original API behavior, but
I think it is okay since it is in the user space.

>         attr.test.repeat = repeat;
>
>         ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> -       if (size_out)
> +       if (data_out)
>                 *size_out = attr.test.data_size_out;
>         if (retval)
>                 *retval = attr.test.retval;
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 2d3c04f45530..560d7527b86b 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -150,6 +150,7 @@ static void test_xdp(void)
>         bpf_map_update_elem(map_fd, &key4, &value4, 0);
>         bpf_map_update_elem(map_fd, &key6, &value6, 0);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
>                                 buf, &size, &retval, &duration);
>
> @@ -158,6 +159,7 @@ static void test_xdp(void)
>               "err %d errno %d retval %d size %d\n",
>               err, errno, retval, size);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != XDP_TX || size != 114 ||
> @@ -182,6 +184,7 @@ static void test_xdp_adjust_tail(void)
>                 return;
>         }
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
>                                 buf, &size, &retval, &duration);
>
> @@ -189,6 +192,7 @@ static void test_xdp_adjust_tail(void)
>               "ipv4", "err %d errno %d retval %d size %d\n",
>               err, errno, retval, size);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != XDP_TX || size != 54,
> @@ -252,6 +256,7 @@ static void test_l4lb(const char *file)
>                 goto out;
>         bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 54 ||
> @@ -259,6 +264,7 @@ static void test_l4lb(const char *file)
>               "err %d errno %d retval %d size %d magic %x\n",
>               err, errno, retval, size, *magic);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 74 ||
> @@ -341,6 +347,7 @@ static void test_xdp_noinline(void)
>                 goto out;
>         bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != 1 || size != 54 ||
> @@ -348,6 +355,7 @@ static void test_xdp_noinline(void)
>               "err %d errno %d retval %d size %d magic %x\n",
>               err, errno, retval, size, *magic);
>
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != 1 || size != 74 ||
> @@ -1795,6 +1803,7 @@ static void test_queue_stack_map(int type)
>                         pkt_v4.iph.saddr = vals[MAP_SIZE - 1 - i] * 5;
>                 }
>
> +               size = sizeof(buf);
>                 err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
>                                         buf, &size, &retval, &duration);
>                 if (err || retval || size != sizeof(pkt_v4) ||
> @@ -1808,6 +1817,7 @@ static void test_queue_stack_map(int type)
>               err, errno, retval, size, iph->daddr);
>
>         /* Queue is empty, program should return TC_ACT_SHOT */
> +       size = sizeof(buf);
>         err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
>                                 buf, &size, &retval, &duration);
>         CHECK(err || retval != 2 /* TC_ACT_SHOT */|| size != sizeof(pkt_v4),
> --
> 2.17.1
>

Re: [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size

[Y Song]

On Fri, Nov 16, 2018 at 12:55 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Make sure that bpf_prog_test_run returns the correct length
> in the size_out argument and that the kernel respects the
> output size hint.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 560d7527b86b..6ab98e10e86f 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -124,6 +124,39 @@ static void test_pkt_access(void)
>         bpf_object__close(obj);
>  }
>
> +static void test_output_size_hint(void)
> +{
> +       const char *file = "./test_pkt_access.o";
> +       struct bpf_object *obj;
> +       __u32 retval, size, duration;
> +       int err, prog_fd;
> +       char buf[10];
> +
> +       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> +       if (err) {
> +               error_cnt++;
> +               return;
> +       }
CHECK can also be used here.
if (CHECK(...)) {
   goto done;
}
where label "done" is right before bpf_object__close.
> +
> +       memset(buf, 0, sizeof(buf));
> +
> +       size = 5;
> +       err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> +                               buf, &size, &retval, &duration);
> +       CHECK(err || retval, "run",
> +             "err %d errno %d retval %d\n",
> +             err, errno, retval);
> +
> +       CHECK(size != sizeof(pkt_v4), "out_size",
> +             "incorrect output size, want %lu have %u\n",
> +             sizeof(pkt_v4), size);
> +
> +       CHECK(buf[5] != 0, "overflow",
> +             "prog_test_run ignored size hint\n");
> +
> +       bpf_object__close(obj);
> +}
> +
>  static void test_xdp(void)
>  {
>         struct vip key4 = {.protocol = 6, .family = AF_INET};
> @@ -1847,6 +1880,7 @@ int main(void)
>         jit_enabled = is_jit_enabled();
>
>         test_pkt_access();
> +       test_output_size_hint();
>         test_xdp();
>         test_xdp_adjust_tail();
>         test_l4lb_all();
> --
> 2.17.1
>

Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface

[Y Song]

On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
> This is because bpf_test_finish copies the output buffer to user space
> without checking its size. This can lead to the kernel overwriting
> data in user space after the buffer if xdp_adjust_head and friends are
> in play.
>
> Fix this by using bpf_attr.test.data_size_out as a size hint. The old
> behaviour is retained if size_hint is zero.

There is a slight change of user space behavior for this patch.
Without this patch, the value bpf_attr.test.data_size_out is output only.
For example,
   output buffer : out_buf (user allocated size 10)
   data_size_out is a random value (e.g., 1),

The actual data to copy is 5.

In today's implementation, the kernel will copy 5 and set data_size_out is 5.

With this patch, the kernel will copy 1 and set data_size_out is 5.

I am not 100% sure at this time whether we CAN overload data_size_out
since it MAY break existing applications.

Alternativley, we can append another field to bpf_attr.test
  __u32 data_out_size;
this will provide the data_out buffer size.
Inside kernel, if the user input attr is smaller than kernel and does not
have data_out_size, the current behavior should be used. Otherwise,
data_out_size is data_out buffer size.

Daniel and Alexei, which option do you think is reasonable?

>
> Interestingly, do_test_single() in test_verifier.c already assumes
> that this is the intended use of data_size_out, and sets it to the
> output buffer size.
>
> Lorenz Bauer (3):
>   bpf: respect size hint to BPF_PROG_TEST_RUN if present
>   libbpf: require size hint in bpf_prog_test_run
>   selftests: add a test for bpf_prog_test_run output size
>
>  net/bpf/test_run.c                       |  9 ++++-
>  tools/lib/bpf/bpf.c                      |  4 ++-
>  tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 2 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface

[Lorenz Bauer]

On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@gmail.com> wrote:
>
> There is a slight change of user space behavior for this patch.
> Without this patch, the value bpf_attr.test.data_size_out is output only.
> For example,
>    output buffer : out_buf (user allocated size 10)
>    data_size_out is a random value (e.g., 1),
>
> The actual data to copy is 5.
>
> In today's implementation, the kernel will copy 5 and set data_size_out is 5.
>
> With this patch, the kernel will copy 1 and set data_size_out is 5.
>
> I am not 100% sure at this time whether we CAN overload data_size_out
> since it MAY break existing applications.

Yes, that's correct. I think that the likelihood of this is low. It would
affect code that uses bpf_attr without zeroing it first. I had a look around,
and I could only find code that would keep working:

* kernel libbpf and descendants (e.g katran)
* github.com/iovisor/gobpf
* github.com/newtools/ebpf

That doesn't really guarantee anything of course.


-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface

[Daniel Borkmann]

On 11/19/2018 03:30 PM, Lorenz Bauer wrote:
> On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@gmail.com> wrote:
>>
>> There is a slight change of user space behavior for this patch.
>> Without this patch, the value bpf_attr.test.data_size_out is output only.
>> For example,
>>    output buffer : out_buf (user allocated size 10)
>>    data_size_out is a random value (e.g., 1),
>>
>> The actual data to copy is 5.
>>
>> In today's implementation, the kernel will copy 5 and set data_size_out is 5.
>>
>> With this patch, the kernel will copy 1 and set data_size_out is 5.
>>
>> I am not 100% sure at this time whether we CAN overload data_size_out
>> since it MAY break existing applications.
> 
> Yes, that's correct. I think that the likelihood of this is low. It would
> affect code that uses bpf_attr without zeroing it first. I had a look around,
> and I could only find code that would keep working:

Agree, it seems like this would be rather unlikely to break the old behavior
and only if some test app forgot to zero it (given data_size_out is also in
the middle and not at the end). I'd rather prefer this approach here and then
push the patch via stable than adding yet another data_size_out-like member.

I think it also makes sense to return a -ENOSPC as Yonghong suggested in order
to indicate to user space that the buffer is not sufficient. Right now this
would have no such indication to the user so it would not be possible to
distinguish whether truncation or not happened. Was thinking whether it makes
sense to indicate through a new flag member that buffer truncation happened,
but I do like -ENOSPC better.

Thanks,
Daniel

Re: [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size

[Lorenz Bauer]

On Sun, 18 Nov 2018 at 05:59, Y Song <ys114321@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 12:55 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Make sure that bpf_prog_test_run returns the correct length
> > in the size_out argument and that the kernel respects the
> > output size hint.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> >  tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 560d7527b86b..6ab98e10e86f 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -124,6 +124,39 @@ static void test_pkt_access(void)
> >         bpf_object__close(obj);
> >  }
> >
> > +static void test_output_size_hint(void)
> > +{
> > +       const char *file = "./test_pkt_access.o";
> > +       struct bpf_object *obj;
> > +       __u32 retval, size, duration;
> > +       int err, prog_fd;
> > +       char buf[10];
> > +
> > +       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > +       if (err) {
> > +               error_cnt++;
> > +               return;
> > +       }
> CHECK can also be used here.
> if (CHECK(...)) {
>    goto done;
> }
> where label "done" is right before bpf_object__close.

I just copied this part from test_pkt_access, happy to change it though.
However, I think "goto done" would lead to freeing an unallocated
object in this case?

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

[PATCH v2 0/4] Fix unsafe BPF_PROG_TEST_RUN interface

[Lorenz Bauer]

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.

Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out

Lorenz Bauer (4):
  bpf: respect size hint to BPF_PROG_TEST_RUN if present
  tools: sync uapi/linux/bpf.h
  libbpf: require size hint in bpf_prog_test_run
  selftests: add a test for bpf_prog_test_run output size

 include/uapi/linux/bpf.h                 |  7 ++--
 net/bpf/test_run.c                       | 15 ++++++--
 tools/include/uapi/linux/bpf.h           |  7 ++--
 tools/lib/bpf/bpf.c                      |  7 +++-
 tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
 5 files changed, 73 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present

[Lorenz Bauer]

Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/uapi/linux/bpf.h |  7 +++++--
 net/bpf/test_run.c       | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 {
 	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
 	int err = -EFAULT;
+	u32 copy_size = size;
 
-	if (data_out && copy_to_user(data_out, data, size))
+	/* Clamp copy if the user has provided a size hint, but copy the full
+	 * buffer if not to retain old behaviour.
+	 */
+	if (kattr->test.data_size_out &&
+	    copy_size > kattr->test.data_size_out) {
+		copy_size = kattr->test.data_size_out;
+		err = -ENOSPC;
+	}
+
+	if (data_out && copy_to_user(data_out, data, copy_size))
 		goto out;
 	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
 		goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 		goto out;
 	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
 		goto out;
-	err = 0;
+	if (err != -ENOSPC)
+		err = 0;
 out:
 	return err;
 }
-- 
2.17.1

[PATCH v2 2/4] tools: sync uapi/linux/bpf.h

[Lorenz Bauer]

Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/include/uapi/linux/bpf.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
-- 
2.17.1

[PATCH v2 3/4] libbpf: require size hint in bpf_prog_test_run

[Lorenz Bauer]

Require size_out to be non-NULL if data_out is given. This prevents
accidental overwriting of process memory after the output buffer.

Adjust callers of bpf_prog_test_run to this behaviour.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/lib/bpf/bpf.c                      |  7 ++++++-
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 961e1b9fc592..1a835ff27486 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	union bpf_attr attr;
 	int ret;
 
+	if (data_out && !size_out)
+		return -EINVAL;
+
 	bzero(&attr, sizeof(attr));
 	attr.test.prog_fd = prog_fd;
 	attr.test.data_in = ptr_to_u64(data);
 	attr.test.data_out = ptr_to_u64(data_out);
 	attr.test.data_size_in = size;
+	if (data_out)
+		attr.test.data_size_out = *size_out;
 	attr.test.repeat = repeat;
 
 	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
-	if (size_out)
+	if (data_out)
 		*size_out = attr.test.data_size_out;
 	if (retval)
 		*retval = attr.test.retval;
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..299938603cb6 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -150,6 +150,7 @@ static void test_xdp(void)
 	bpf_map_update_elem(map_fd, &key4, &value4, 0);
 	bpf_map_update_elem(map_fd, &key6, &value6, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 
@@ -158,6 +159,7 @@ static void test_xdp(void)
 	      "err %d errno %d retval %d size %d\n",
 	      err, errno, retval, size);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != XDP_TX || size != 114 ||
@@ -182,6 +184,7 @@ static void test_xdp_adjust_tail(void)
 		return;
 	}
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 
@@ -189,6 +192,7 @@ static void test_xdp_adjust_tail(void)
 	      "ipv4", "err %d errno %d retval %d size %d\n",
 	      err, errno, retval, size);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != XDP_TX || size != 54,
@@ -252,6 +256,7 @@ static void test_l4lb(const char *file)
 		goto out;
 	bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 54 ||
@@ -259,6 +264,7 @@ static void test_l4lb(const char *file)
 	      "err %d errno %d retval %d size %d magic %x\n",
 	      err, errno, retval, size, *magic);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 74 ||
@@ -341,6 +347,7 @@ static void test_xdp_noinline(void)
 		goto out;
 	bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 1 || size != 54 ||
@@ -348,6 +355,7 @@ static void test_xdp_noinline(void)
 	      "err %d errno %d retval %d size %d magic %x\n",
 	      err, errno, retval, size, *magic);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 1 || size != 74 ||
@@ -1795,6 +1803,7 @@ static void test_queue_stack_map(int type)
 			pkt_v4.iph.saddr = vals[MAP_SIZE - 1 - i] * 5;
 		}
 
+		size = sizeof(buf);
 		err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 					buf, &size, &retval, &duration);
 		if (err || retval || size != sizeof(pkt_v4) ||
@@ -1808,6 +1817,7 @@ static void test_queue_stack_map(int type)
 	      err, errno, retval, size, iph->daddr);
 
 	/* Queue is empty, program should return TC_ACT_SHOT */
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 	CHECK(err || retval != 2 /* TC_ACT_SHOT */|| size != sizeof(pkt_v4),
-- 
2.17.1

[PATCH v2 4/4] selftests: add a test for bpf_prog_test_run output size

[Lorenz Bauer]

Make sure that bpf_prog_test_run returns the correct length
in the size_out argument and that the kernel respects the
output size hint. Also check that errno indicates ENOSPC.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 299938603cb6..92e48c2ba2c6 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -124,6 +124,39 @@ static void test_pkt_access(void)
 	bpf_object__close(obj);
 }
 
+static void test_output_size_hint(void)
+{
+	const char *file = "./test_pkt_access.o";
+	struct bpf_object *obj;
+	__u32 retval, size, duration;
+	int err, prog_fd;
+	char buf[10];
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+	if (CHECK(err, "load", "err %d errno %d\n", err, errno))
+		return;
+
+	memset(buf, 0, sizeof(buf));
+
+	size = 5;
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				buf, &size, &retval, &duration);
+	CHECK(err != -1 || errno != ENOSPC || retval, "run",
+	      "err %d errno %d retval %d\n",
+	      err, errno, retval);
+
+	duration = 0;
+
+	CHECK(size != sizeof(pkt_v4), "out_size",
+	      "incorrect output size, want %lu have %u\n",
+	      sizeof(pkt_v4), size);
+
+	CHECK(buf[5] != 0, "overflow",
+	      "prog_test_run ignored size hint\n");
+
+	bpf_object__close(obj);
+}
+
 static void test_xdp(void)
 {
 	struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1847,6 +1880,7 @@ int main(void)
 	jit_enabled = is_jit_enabled();
 
 	test_pkt_access();
+	test_output_size_hint();
 	test_xdp();
 	test_xdp_adjust_tail();
 	test_l4lb_all();
-- 
2.17.1

Re: [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size

[Y Song]

On Tue, Nov 20, 2018 at 3:35 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Sun, 18 Nov 2018 at 05:59, Y Song <ys114321@gmail.com> wrote:
> >
> > On Fri, Nov 16, 2018 at 12:55 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > Make sure that bpf_prog_test_run returns the correct length
> > > in the size_out argument and that the kernel respects the
> > > output size hint.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > ---
> > >  tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index 560d7527b86b..6ab98e10e86f 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -124,6 +124,39 @@ static void test_pkt_access(void)
> > >         bpf_object__close(obj);
> > >  }
> > >
> > > +static void test_output_size_hint(void)
> > > +{
> > > +       const char *file = "./test_pkt_access.o";
> > > +       struct bpf_object *obj;
> > > +       __u32 retval, size, duration;
> > > +       int err, prog_fd;
> > > +       char buf[10];
> > > +
> > > +       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > > +       if (err) {
> > > +               error_cnt++;
> > > +               return;
> > > +       }
> > CHECK can also be used here.
> > if (CHECK(...)) {
> >    goto done;
> > }
> > where label "done" is right before bpf_object__close.
>
> I just copied this part from test_pkt_access, happy to change it though.
> However, I think "goto done" would lead to freeing an unallocated
> object in this case?

Right, you can just return here.

>
> --
> Lorenz Bauer  |  Systems Engineer
> 25 Lavington St., London SE1 0NZ
>
> www.cloudflare.com

Re: [PATCH v2 0/4] Fix unsafe BPF_PROG_TEST_RUN interface

[Y Song]

On Tue, Nov 20, 2018 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
> This is because bpf_test_finish copies the output buffer to user space
> without checking its size. This can lead to the kernel overwriting
> data in user space after the buffer if xdp_adjust_head and friends are
> in play.
>
> Changes in v2:
> * Make the syscall return ENOSPC if data_size_out is too small
> * Make bpf_prog_test_run return EINVAL if size_out is missing
> * Document the new behaviour of data_size_out
>
> Lorenz Bauer (4):
>   bpf: respect size hint to BPF_PROG_TEST_RUN if present
>   tools: sync uapi/linux/bpf.h
>   libbpf: require size hint in bpf_prog_test_run
>   selftests: add a test for bpf_prog_test_run output size

For the series, if we decided to take this approach rather than
amending another field
in the uapi as described in https://www.spinics.net/lists/netdev/msg534277.html,
then
  Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH v2 3/4] libbpf: require size hint in bpf_prog_test_run

[Alexei Starovoitov]

On Tue, Nov 20, 2018 at 03:43:05PM +0000, Lorenz Bauer wrote:
> Require size_out to be non-NULL if data_out is given. This prevents
> accidental overwriting of process memory after the output buffer.
> 
> Adjust callers of bpf_prog_test_run to this behaviour.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/lib/bpf/bpf.c                      |  7 ++++++-
>  tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 961e1b9fc592..1a835ff27486 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
>  	union bpf_attr attr;
>  	int ret;
>  
> +	if (data_out && !size_out)
> +		return -EINVAL;
> +
>  	bzero(&attr, sizeof(attr));
>  	attr.test.prog_fd = prog_fd;
>  	attr.test.data_in = ptr_to_u64(data);
>  	attr.test.data_out = ptr_to_u64(data_out);
>  	attr.test.data_size_in = size;
> +	if (data_out)
> +		attr.test.data_size_out = *size_out;
>  	attr.test.repeat = repeat;
>  
>  	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> -	if (size_out)
> +	if (data_out)
>  		*size_out = attr.test.data_size_out;
>  	if (retval)
>  		*retval = attr.test.retval;
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index c1e688f61061..299938603cb6 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -150,6 +150,7 @@ static void test_xdp(void)
>  	bpf_map_update_elem(map_fd, &key4, &value4, 0);
>  	bpf_map_update_elem(map_fd, &key6, &value6, 0);
>  
> +	size = sizeof(buf);
>  	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
>  				buf, &size, &retval, &duration);
>  
> @@ -158,6 +159,7 @@ static void test_xdp(void)
>  	      "err %d errno %d retval %d size %d\n",
>  	      err, errno, retval, size);
>  
> +	size = sizeof(buf);
>  	err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
>  				buf, &size, &retval, &duration);

This will surely break existing bpf_prog_test_run users.
Like it will break our testing framework.
we can fix out stuff and libbpf is a user space library, but I don't
think that this is the case to invoke such pain.
libbpf's bpf_prog_test_run() should be a simple wrapper on top of syscall.
I don't think it should be making such restrictions on api.

btw patch 1 looks good to me.

Re: [PATCH v2 3/4] libbpf: require size hint in bpf_prog_test_run

[Lorenz Bauer]

On Tue, 20 Nov 2018 at 19:18, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 20, 2018 at 03:43:05PM +0000, Lorenz Bauer wrote:
> > Require size_out to be non-NULL if data_out is given. This prevents
> > accidental overwriting of process memory after the output buffer.
> >
> > Adjust callers of bpf_prog_test_run to this behaviour.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> >  tools/lib/bpf/bpf.c                      |  7 ++++++-
> >  tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 961e1b9fc592..1a835ff27486 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> >       union bpf_attr attr;
> >       int ret;
> >
> > +     if (data_out && !size_out)
> > +             return -EINVAL;
> > +
> >       bzero(&attr, sizeof(attr));
> >       attr.test.prog_fd = prog_fd;
> >       attr.test.data_in = ptr_to_u64(data);
> >       attr.test.data_out = ptr_to_u64(data_out);
> >       attr.test.data_size_in = size;
> > +     if (data_out)
> > +             attr.test.data_size_out = *size_out;
> >       attr.test.repeat = repeat;
> >
> >       ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> > -     if (size_out)
> > +     if (data_out)
> >               *size_out = attr.test.data_size_out;
> >       if (retval)
> >               *retval = attr.test.retval;
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index c1e688f61061..299938603cb6 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -150,6 +150,7 @@ static void test_xdp(void)
> >       bpf_map_update_elem(map_fd, &key4, &value4, 0);
> >       bpf_map_update_elem(map_fd, &key6, &value6, 0);
> >
> > +     size = sizeof(buf);
> >       err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> >                               buf, &size, &retval, &duration);
> >
> > @@ -158,6 +159,7 @@ static void test_xdp(void)
> >             "err %d errno %d retval %d size %d\n",
> >             err, errno, retval, size);
> >
> > +     size = sizeof(buf);
> >       err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
> >                               buf, &size, &retval, &duration);
>
> This will surely break existing bpf_prog_test_run users.
> Like it will break our testing framework.
> we can fix out stuff and libbpf is a user space library, but I don't
> think that this is the case to invoke such pain.
> libbpf's bpf_prog_test_run() should be a simple wrapper on top of syscall.
> I don't think it should be making such restrictions on api.
>
> btw patch 1 looks good to me.
>

What if I add bpf_prog_test_run_safe or similar, with the behaviour
proposed in the patch?
Makes sense that you don't want to break existing users of libbpf
outside the kernel, OTOH
user space really should specify the output buffer length (or be given
the choice).

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

Re: [PATCH v2 3/4] libbpf: require size hint in bpf_prog_test_run

[Alexei Starovoitov]

On Tue, Nov 20, 2018 at 07:43:57PM +0000, Lorenz Bauer wrote:
> On Tue, 20 Nov 2018 at 19:18, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Nov 20, 2018 at 03:43:05PM +0000, Lorenz Bauer wrote:
> > > Require size_out to be non-NULL if data_out is given. This prevents
> > > accidental overwriting of process memory after the output buffer.
> > >
> > > Adjust callers of bpf_prog_test_run to this behaviour.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > ---
> > >  tools/lib/bpf/bpf.c                      |  7 ++++++-
> > >  tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
> > >  2 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > index 961e1b9fc592..1a835ff27486 100644
> > > --- a/tools/lib/bpf/bpf.c
> > > +++ b/tools/lib/bpf/bpf.c
> > > @@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> > >       union bpf_attr attr;
> > >       int ret;
> > >
> > > +     if (data_out && !size_out)
> > > +             return -EINVAL;
> > > +
> > >       bzero(&attr, sizeof(attr));
> > >       attr.test.prog_fd = prog_fd;
> > >       attr.test.data_in = ptr_to_u64(data);
> > >       attr.test.data_out = ptr_to_u64(data_out);
> > >       attr.test.data_size_in = size;
> > > +     if (data_out)
> > > +             attr.test.data_size_out = *size_out;
> > >       attr.test.repeat = repeat;
> > >
> > >       ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> > > -     if (size_out)
> > > +     if (data_out)
> > >               *size_out = attr.test.data_size_out;
> > >       if (retval)
> > >               *retval = attr.test.retval;
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index c1e688f61061..299938603cb6 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -150,6 +150,7 @@ static void test_xdp(void)
> > >       bpf_map_update_elem(map_fd, &key4, &value4, 0);
> > >       bpf_map_update_elem(map_fd, &key6, &value6, 0);
> > >
> > > +     size = sizeof(buf);
> > >       err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> > >                               buf, &size, &retval, &duration);
> > >
> > > @@ -158,6 +159,7 @@ static void test_xdp(void)
> > >             "err %d errno %d retval %d size %d\n",
> > >             err, errno, retval, size);
> > >
> > > +     size = sizeof(buf);
> > >       err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
> > >                               buf, &size, &retval, &duration);
> >
> > This will surely break existing bpf_prog_test_run users.
> > Like it will break our testing framework.
> > we can fix out stuff and libbpf is a user space library, but I don't
> > think that this is the case to invoke such pain.
> > libbpf's bpf_prog_test_run() should be a simple wrapper on top of syscall.
> > I don't think it should be making such restrictions on api.
> >
> > btw patch 1 looks good to me.
> >
> 
> What if I add bpf_prog_test_run_safe or similar, with the behaviour
> proposed in the patch?
> Makes sense that you don't want to break existing users of libbpf
> outside the kernel, OTOH
> user space really should specify the output buffer length (or be given
> the choice).

+     if (data_out && !size_out)
+             return -EINVAL;
+
+     if (data_out)
+             attr.test.data_size_out = *size_out;

this is actually worse than I thought, since it will cause sporadic
failures in the test frameworks that don't init size_out.
Like test_progs.c will be randomly passing/failing depending
on the state of uninit bytes in the stack.

Also consider that during bpf uconf folks have requested to extend
prog_test_run with __sk_buff in/out argument, so no only packet data,
but skb related fields can be tested as well.
I think that was a valid request and prog_test_run should be extended.
So soon such libbpf's bpf_prog_test_run_safe() will not be enough.
I think it's the best to use _xattr approach we did for map_create
and prog_load.
This new bpf_prog_test_run_xattr() will be able to do the check
you're proposing:
+     if (data_out && !size_out)
+             return -EINVAL;
+
+     if (data_out)
+             attr.test.data_size_out = *size_out;
it can also check that both size and size_out are sane
with similar check to kernel:
if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom);

and will be extendable in the near future with __sk_buff in/out.

[PATCH v3 0/4] Fix unsafe BPF_PROG_TEST_RUN interface

[Lorenz Bauer]

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.

Changes in v3:
* Introduce bpf_prog_test_run_xattr instead of modifying the existing
  function

Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out

Lorenz Bauer (4):
  bpf: respect size hint to BPF_PROG_TEST_RUN if present
  tools: sync uapi/linux/bpf.h
  libbpf: add bpf_prog_test_run_xattr
  selftests: add a test for bpf_prog_test_run_xattr

 include/uapi/linux/bpf.h                 |  7 +++-
 net/bpf/test_run.c                       | 15 +++++++-
 tools/include/uapi/linux/bpf.h           |  7 +++-
 tools/lib/bpf/bpf.c                      | 27 +++++++++++++
 tools/lib/bpf/bpf.h                      | 13 +++++++
 tools/testing/selftests/bpf/test_progs.c | 49 ++++++++++++++++++++++++
 6 files changed, 112 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present

[Lorenz Bauer]

Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/uapi/linux/bpf.h |  7 +++++--
 net/bpf/test_run.c       | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 {
 	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
 	int err = -EFAULT;
+	u32 copy_size = size;
 
-	if (data_out && copy_to_user(data_out, data, size))
+	/* Clamp copy if the user has provided a size hint, but copy the full
+	 * buffer if not to retain old behaviour.
+	 */
+	if (kattr->test.data_size_out &&
+	    copy_size > kattr->test.data_size_out) {
+		copy_size = kattr->test.data_size_out;
+		err = -ENOSPC;
+	}
+
+	if (data_out && copy_to_user(data_out, data, copy_size))
 		goto out;
 	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
 		goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 		goto out;
 	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
 		goto out;
-	err = 0;
+	if (err != -ENOSPC)
+		err = 0;
 out:
 	return err;
 }
-- 
2.17.1

[PATCH v3 2/4] tools: sync uapi/linux/bpf.h

[Lorenz Bauer]

Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/include/uapi/linux/bpf.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
-- 
2.17.1

[PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Lorenz Bauer]

Add a new function, which encourages safe usage of the test interface.
bpf_prog_test_run continues to work as before, but should be considered
unsafe.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
 tools/lib/bpf/bpf.h | 13 +++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 961e1b9fc592..f8518bef6886 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	return ret;
 }
 
+int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+			    __u32 *size_out, __u32 *retval, __u32 *duration)
+{
+	union bpf_attr attr;
+	int ret;
+
+	if (!test_attr->data_out && test_attr->size_out > 0)
+		return -EINVAL;
+
+	bzero(&attr, sizeof(attr));
+	attr.test.prog_fd = test_attr->prog_fd;
+	attr.test.data_in = ptr_to_u64(test_attr->data);
+	attr.test.data_out = ptr_to_u64(test_attr->data_out);
+	attr.test.data_size_in = test_attr->size;
+	attr.test.data_size_out = test_attr->size_out;
+	attr.test.repeat = test_attr->repeat;
+
+	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+	if (size_out)
+		*size_out = attr.test.data_size_out;
+	if (retval)
+		*retval = attr.test.retval;
+	if (duration)
+		*duration = attr.test.duration;
+	return ret;
+}
+
 int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
 {
 	union bpf_attr attr;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 26a51538213c..570f19f77f42 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
 LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
 				enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+	int prog_fd;
+	int repeat;
+	const void *data;
+	__u32 size;
+	void *data_out; /* optional */
+	__u32 size_out;
+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+				       __u32 *size_out, __u32 *retval,
+				       __u32 *duration);
 LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
 				 __u32 size, void *data_out, __u32 *size_out,
 				 __u32 *retval, __u32 *duration);
-- 
2.17.1

[PATCH v3 4/4] selftests: add a test for bpf_prog_test_run_xattr

[Lorenz Bauer]

Make sure that bpf_prog_test_run_xattr returns the correct length
and that the kernel respects the output size hint. Also check
that errno indicates ENOSPC if there is a short output buffer given.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/test_progs.c | 49 ++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..f9f5b1dbcc83 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -124,6 +124,54 @@ static void test_pkt_access(void)
 	bpf_object__close(obj);
 }
 
+static void test_prog_run_xattr(void)
+{
+	const char *file = "./test_pkt_access.o";
+	__u32 duration, retval, size_out;
+	struct bpf_object *obj;
+	char buf[10];
+	int err;
+	struct bpf_prog_test_run_attr tattr = {
+		.repeat = 1,
+		.data = &pkt_v4,
+		.size = sizeof(pkt_v4),
+		.data_out = buf,
+		.size_out = 5,
+	};
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj,
+			    &tattr.prog_fd);
+	if (CHECK(err, "load", "err %d errno %d\n", err, errno))
+		return;
+
+	memset(buf, 0, sizeof(buf));
+
+	err = bpf_prog_test_run_xattr(&tattr, &size_out, &retval, &duration);
+	CHECK(err != -1 || errno != ENOSPC || retval, "run",
+	      "err %d errno %d retval %d\n", err, errno, retval);
+
+	CHECK(size_out != sizeof(pkt_v4), "output_size",
+	      "incorrect output size, want %lu have %u\n",
+	      sizeof(pkt_v4), size_out);
+
+	CHECK(buf[5] != 0, "overflow",
+	      "BPF_PROG_TEST_RUN ignored size hint\n");
+
+	tattr.data_out = NULL;
+	tattr.size_out = 0;
+	errno = 0;
+
+	err = bpf_prog_test_run_xattr(&tattr, NULL, &retval, &duration);
+	CHECK(err || errno || retval, "run_no_output",
+	      "err %d errno %d retval %d\n", err, errno, retval);
+
+	tattr.size_out = 1;
+	err = bpf_prog_test_run_xattr(&tattr, NULL, NULL, &duration);
+	CHECK(err != -EINVAL, "run_wrong_size_out", "err %d\n", err);
+
+	bpf_object__close(obj);
+}
+
 static void test_xdp(void)
 {
 	struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1837,6 +1885,7 @@ int main(void)
 	jit_enabled = is_jit_enabled();
 
 	test_pkt_access();
+	test_prog_run_xattr();
 	test_xdp();
 	test_xdp_adjust_tail();
 	test_l4lb_all();
-- 
2.17.1

Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Daniel Borkmann]

On 11/22/2018 03:09 PM, Lorenz Bauer wrote:
> Add a new function, which encourages safe usage of the test interface.
> bpf_prog_test_run continues to work as before, but should be considered
> unsafe.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>

Set looks good to me, thanks! Three small things below:

> ---
>  tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
>  tools/lib/bpf/bpf.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 961e1b9fc592..f8518bef6886 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
>  	return ret;
>  }
>  
> +int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
> +			    __u32 *size_out, __u32 *retval, __u32 *duration)
> +{
> +	union bpf_attr attr;
> +	int ret;
> +
> +	if (!test_attr->data_out && test_attr->size_out > 0)
> +		return -EINVAL;
> +
> +	bzero(&attr, sizeof(attr));
> +	attr.test.prog_fd = test_attr->prog_fd;
> +	attr.test.data_in = ptr_to_u64(test_attr->data);
> +	attr.test.data_out = ptr_to_u64(test_attr->data_out);
> +	attr.test.data_size_in = test_attr->size;
> +	attr.test.data_size_out = test_attr->size_out;
> +	attr.test.repeat = test_attr->repeat;
> +
> +	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> +	if (size_out)
> +		*size_out = attr.test.data_size_out;
> +	if (retval)
> +		*retval = attr.test.retval;
> +	if (duration)
> +		*duration = attr.test.duration;
> +	return ret;
> +}
> +
>  int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
>  {
>  	union bpf_attr attr;
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 26a51538213c..570f19f77f42 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
>  LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
>  LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
>  				enum bpf_attach_type type);
> +
> +struct bpf_prog_test_run_attr {
> +	int prog_fd;
> +	int repeat;
> +	const void *data;
> +	__u32 size;
> +	void *data_out; /* optional */
> +	__u32 size_out;

Small nit: could we name these data_{in,out} and data_size_{in,out} as
well, so it's analog to the ones from the bpf_attr?

> +};
> +
> +LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
> +				       __u32 *size_out, __u32 *retval,
> +				       __u32 *duration);
>  LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
>  				 __u32 size, void *data_out, __u32 *size_out,
>  				 __u32 *retval, __u32 *duration);

Could we add a comment into the header here stating that we discourage
bpf_prog_test_run()'s use?

It would probably also make sense since we go that route that we would
convert the 10 bpf_prog_test_run() instances under test_progs.c at the
same time so that people extending or looking at BPF kselftests don't
copy discouraged bpf_prog_test_run() api as examples from this point
onwards anymore.

Thanks,
Daniel

Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Alexei Starovoitov]

On Fri, Nov 23, 2018 at 11:25:11PM +0100, Daniel Borkmann wrote:
> On 11/22/2018 03:09 PM, Lorenz Bauer wrote:
> > Add a new function, which encourages safe usage of the test interface.
> > bpf_prog_test_run continues to work as before, but should be considered
> > unsafe.
> > 
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> 
> Set looks good to me, thanks! Three small things below:
> 
> > ---
> >  tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
> >  tools/lib/bpf/bpf.h | 13 +++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 961e1b9fc592..f8518bef6886 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> >  	return ret;
> >  }
> >  
> > +int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
> > +			    __u32 *size_out, __u32 *retval, __u32 *duration)
> > +{
> > +	union bpf_attr attr;
> > +	int ret;
> > +
> > +	if (!test_attr->data_out && test_attr->size_out > 0)
> > +		return -EINVAL;
> > +
> > +	bzero(&attr, sizeof(attr));
> > +	attr.test.prog_fd = test_attr->prog_fd;
> > +	attr.test.data_in = ptr_to_u64(test_attr->data);
> > +	attr.test.data_out = ptr_to_u64(test_attr->data_out);
> > +	attr.test.data_size_in = test_attr->size;
> > +	attr.test.data_size_out = test_attr->size_out;
> > +	attr.test.repeat = test_attr->repeat;
> > +
> > +	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> > +	if (size_out)
> > +		*size_out = attr.test.data_size_out;
> > +	if (retval)
> > +		*retval = attr.test.retval;
> > +	if (duration)
> > +		*duration = attr.test.duration;
> > +	return ret;
> > +}
> > +
> >  int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
> >  {
> >  	union bpf_attr attr;
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 26a51538213c..570f19f77f42 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
> >  LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
> >  LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
> >  				enum bpf_attach_type type);
> > +
> > +struct bpf_prog_test_run_attr {
> > +	int prog_fd;
> > +	int repeat;
> > +	const void *data;
> > +	__u32 size;
> > +	void *data_out; /* optional */
> > +	__u32 size_out;
> 
> Small nit: could we name these data_{in,out} and data_size_{in,out} as
> well, so it's analog to the ones from the bpf_attr?
> 
> > +};
> > +
> > +LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
> > +				       __u32 *size_out, __u32 *retval,
> > +				       __u32 *duration);

can we keep return values in struct bpf_prog_test_run_attr ?
I think the interface will be easier to use and faster. Less pointers
to pass around.

> >  LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
> >  				 __u32 size, void *data_out, __u32 *size_out,
> >  				 __u32 *retval, __u32 *duration);
> 
> Could we add a comment into the header here stating that we discourage
> bpf_prog_test_run()'s use?
> 
> It would probably also make sense since we go that route that we would
> convert the 10 bpf_prog_test_run() instances under test_progs.c at the
> same time so that people extending or looking at BPF kselftests don't
> copy discouraged bpf_prog_test_run() api as examples from this point
> onwards anymore.

I would keep bpf_prog_test_run() and test_progs.c as-is.
I think the prog_test_run in the current form is actually fine.
I don't find it 'unsafe'.
When it's called on a given bpf prog the user knows what prog
suppose to do. If prog is increasing the packet size the test writer
knows that and should size the output buffer accordingly.
Also there are plenty of tests where progs don't change the packet size
and any test with 'repeat > 1' should have the packet size
return to initial value. Like if the test is doing ipip encap
at the end of the run the bpf prog should remove that encap.
Otherwise 'repeat > 1' will produce wrong results.

Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Lorenz Bauer]

On Sat, 24 Nov 2018 at 22:20, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Nov 23, 2018 at 11:25:11PM +0100, Daniel Borkmann wrote:
> > On 11/22/2018 03:09 PM, Lorenz Bauer wrote:
> > > Add a new function, which encourages safe usage of the test interface.
> > > bpf_prog_test_run continues to work as before, but should be considered
> > > unsafe.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> >
> > Set looks good to me, thanks! Three small things below:
> >
> > > ---
> > >  tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
> > >  tools/lib/bpf/bpf.h | 13 +++++++++++++
> > >  2 files changed, 40 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > index 961e1b9fc592..f8518bef6886 100644
> > > --- a/tools/lib/bpf/bpf.c
> > > +++ b/tools/lib/bpf/bpf.c
> > > @@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> > >     return ret;
> > >  }
> > >
> > > +int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
> > > +                       __u32 *size_out, __u32 *retval, __u32 *duration)
> > > +{
> > > +   union bpf_attr attr;
> > > +   int ret;
> > > +
> > > +   if (!test_attr->data_out && test_attr->size_out > 0)
> > > +           return -EINVAL;
> > > +
> > > +   bzero(&attr, sizeof(attr));
> > > +   attr.test.prog_fd = test_attr->prog_fd;
> > > +   attr.test.data_in = ptr_to_u64(test_attr->data);
> > > +   attr.test.data_out = ptr_to_u64(test_attr->data_out);
> > > +   attr.test.data_size_in = test_attr->size;
> > > +   attr.test.data_size_out = test_attr->size_out;
> > > +   attr.test.repeat = test_attr->repeat;
> > > +
> > > +   ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> > > +   if (size_out)
> > > +           *size_out = attr.test.data_size_out;
> > > +   if (retval)
> > > +           *retval = attr.test.retval;
> > > +   if (duration)
> > > +           *duration = attr.test.duration;
> > > +   return ret;
> > > +}
> > > +
> > >  int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
> > >  {
> > >     union bpf_attr attr;
> > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > > index 26a51538213c..570f19f77f42 100644
> > > --- a/tools/lib/bpf/bpf.h
> > > +++ b/tools/lib/bpf/bpf.h
> > > @@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
> > >  LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
> > >  LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
> > >                             enum bpf_attach_type type);
> > > +
> > > +struct bpf_prog_test_run_attr {
> > > +   int prog_fd;
> > > +   int repeat;
> > > +   const void *data;
> > > +   __u32 size;
> > > +   void *data_out; /* optional */
> > > +   __u32 size_out;
> >
> > Small nit: could we name these data_{in,out} and data_size_{in,out} as
> > well, so it's analog to the ones from the bpf_attr?
> >
> > > +};
> > > +
> > > +LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
> > > +                                  __u32 *size_out, __u32 *retval,
> > > +                                  __u32 *duration);
>
> can we keep return values in struct bpf_prog_test_run_attr ?
> I think the interface will be easier to use and faster. Less pointers
> to pass around.

That's what I had initially, but that makes re-using test_attr really
awkward. Either
you need to reset data_out_size before every call because it is used
to return the
buffer size, or we need another member in test_attr specifically for this. Then
naming becomes really awkward (data_size_out_out anyone?). It also means
we can't take a const struct attr, which is contrary to the other
xattr functions which
already exist.

I think actually inspecting the required size of the output buffer
will be a rare
occurrence, so making the user jump through the hoop of a pointer doesn't seem
too onerous.

>
> > >  LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
> > >                              __u32 size, void *data_out, __u32 *size_out,
> > >                              __u32 *retval, __u32 *duration);
> >
> > Could we add a comment into the header here stating that we discourage
> > bpf_prog_test_run()'s use?
> >
> > It would probably also make sense since we go that route that we would
> > convert the 10 bpf_prog_test_run() instances under test_progs.c at the
> > same time so that people extending or looking at BPF kselftests don't
> > copy discouraged bpf_prog_test_run() api as examples from this point
> > onwards anymore.
>
> I would keep bpf_prog_test_run() and test_progs.c as-is.
> I think the prog_test_run in the current form is actually fine.
> I don't find it 'unsafe'.
> When it's called on a given bpf prog the user knows what prog
> suppose to do. If prog is increasing the packet size the test writer
> knows that and should size the output buffer accordingly.

I guess this is a matter of perspective. I prefer an interface that
gives me back
an error message, rather than corrupt my stack / heap, when the assumptions
change. It's also nicer to use from "managed" languages like Go where users
aren't as accustomed to issues like these.

Do you object to me adding the disclaimer to the header as Daniel suggested?

> Also there are plenty of tests where progs don't change the packet size
> and any test with 'repeat > 1' should have the packet size
> return to initial value. Like if the test is doing ipip encap
> at the end of the run the bpf prog should remove that encap.
> Otherwise 'repeat > 1' will produce wrong results.
>

Yup. Another thorny part of the test interface, which I'd like to improve! :)
Don't know how to do it yet though.

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Daniel Borkmann]

On 11/26/2018 01:45 PM, Lorenz Bauer wrote:
> On Sat, 24 Nov 2018 at 22:20, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Fri, Nov 23, 2018 at 11:25:11PM +0100, Daniel Borkmann wrote:
>>> On 11/22/2018 03:09 PM, Lorenz Bauer wrote:
[...]
>>>>  LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
>>>>                              __u32 size, void *data_out, __u32 *size_out,
>>>>                              __u32 *retval, __u32 *duration);
>>>
>>> Could we add a comment into the header here stating that we discourage
>>> bpf_prog_test_run()'s use?
>>>
>>> It would probably also make sense since we go that route that we would
>>> convert the 10 bpf_prog_test_run() instances under test_progs.c at the
>>> same time so that people extending or looking at BPF kselftests don't
>>> copy discouraged bpf_prog_test_run() api as examples from this point
>>> onwards anymore.
>>
>> I would keep bpf_prog_test_run() and test_progs.c as-is.
>> I think the prog_test_run in the current form is actually fine.
>> I don't find it 'unsafe'.
>> When it's called on a given bpf prog the user knows what prog
>> suppose to do. If prog is increasing the packet size the test writer
>> knows that and should size the output buffer accordingly.
> 
> I guess this is a matter of perspective. I prefer an interface that
> gives me back
> an error message, rather than corrupt my stack / heap, when the assumptions
> change. It's also nicer to use from "managed" languages like Go where users
> aren't as accustomed to issues like these.
> 
> Do you object to me adding the disclaimer to the header as Daniel suggested?

Agree that prog_test_run() in the current form is actually fine, I was mostly
thinking that it may be non-obvious to users extending the tests or writing
their own test framework and checking how BPF kselftests does it (since it's
kind of a prime example), so they might end up using the wrong API and run
into mentioned issues w/o realizing. At min some comment with more context
would be needed.

>> Also there are plenty of tests where progs don't change the packet size
>> and any test with 'repeat > 1' should have the packet size
>> return to initial value. Like if the test is doing ipip encap
>> at the end of the run the bpf prog should remove that encap.
>> Otherwise 'repeat > 1' will produce wrong results.
> 
> Yup. Another thorny part of the test interface, which I'd like to improve! :)
> Don't know how to do it yet though.

Yep, somehow here we would need to restore original input packet layout/data
so that the test program always runs on the user defined one.

Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Alexei Starovoitov]

On Mon, Nov 26, 2018 at 4:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> That's what I had initially, but that makes re-using test_attr really
> awkward. Either
> you need to reset data_out_size before every call because it is used
> to return the
> buffer size,

I think that is exactly what the user of the interface would want to do.
Why would anyone keep reusing the same test_attr on multiple calls
into the kernel without changing the fields?

> It also means
> we can't take a const struct attr, which is contrary to the other
> xattr functions which
> already exist.

I don't see an issue with that.

> I think actually inspecting the required size of the output buffer
> will be a rare
> occurrence, so making the user jump through the hoop of a pointer doesn't seem
> too onerous.

I think the opposite is the case.
If the output buffer is provided the test will be comparing it
to expected value.

Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

[Lorenz Bauer]

On Wed, 28 Nov 2018 at 05:05, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 26, 2018 at 4:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > That's what I had initially, but that makes re-using test_attr really
> > awkward. Either
> > you need to reset data_out_size before every call because it is used
> > to return the
> > buffer size,
>
> I think that is exactly what the user of the interface would want to do.
> Why would anyone keep reusing the same test_attr on multiple calls
> into the kernel without changing the fields?

Basically, you can only change the input part without having to reset
data_size_out to sizeof(buffer). Not a big deal, I'll change it.

>
> > It also means
> > we can't take a const struct attr, which is contrary to the other
> > xattr functions which
> > already exist.
>
> I don't see an issue with that.
>
> > I think actually inspecting the required size of the output buffer
> > will be a rare
> > occurrence, so making the user jump through the hoop of a pointer doesn't seem
> > too onerous.
>
> I think the opposite is the case.
> If the output buffer is provided the test will be comparing it
> to expected value.

Yeah, I wasn't thinking too hard on this one, sorry. User space needs to check
where the end of the buffer is.

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

[PATCH v4 0/4] Fix unsafe BPF_PROG_TEST_RUN interface

[Lorenz Bauer]

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.

Changes in v4:
* Document bpf_prog_test_run and bpf_prog_test_run_xattr
* Use struct bpf_prog_test_run_attr for return values

Changes in v3:
* Introduce bpf_prog_test_run_xattr instead of modifying the existing
  function

Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out

Lorenz Bauer (4):
  bpf: respect size hint to BPF_PROG_TEST_RUN if present
  tools: sync uapi/linux/bpf.h
  libbpf: add bpf_prog_test_run_xattr
  selftests: add a test for bpf_prog_test_run_xattr

 include/uapi/linux/bpf.h                 |  7 ++-
 net/bpf/test_run.c                       | 15 ++++++-
 tools/include/uapi/linux/bpf.h           |  7 ++-
 tools/lib/bpf/bpf.c                      | 23 ++++++++++
 tools/lib/bpf/bpf.h                      | 19 ++++++++
 tools/testing/selftests/bpf/test_progs.c | 55 +++++++++++++++++++++++-
 6 files changed, 119 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v4 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present

[Lorenz Bauer]

Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/uapi/linux/bpf.h |  7 +++++--
 net/bpf/test_run.c       | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 {
 	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
 	int err = -EFAULT;
+	u32 copy_size = size;
 
-	if (data_out && copy_to_user(data_out, data, size))
+	/* Clamp copy if the user has provided a size hint, but copy the full
+	 * buffer if not to retain old behaviour.
+	 */
+	if (kattr->test.data_size_out &&
+	    copy_size > kattr->test.data_size_out) {
+		copy_size = kattr->test.data_size_out;
+		err = -ENOSPC;
+	}
+
+	if (data_out && copy_to_user(data_out, data, copy_size))
 		goto out;
 	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
 		goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 		goto out;
 	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
 		goto out;
-	err = 0;
+	if (err != -ENOSPC)
+		err = 0;
 out:
 	return err;
 }
-- 
2.17.1

[PATCH v4 2/4] tools: sync uapi/linux/bpf.h

[Lorenz Bauer]

Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/include/uapi/linux/bpf.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
-- 
2.17.1

[PATCH v4 3/4] libbpf: add bpf_prog_test_run_xattr

[Lorenz Bauer]

Add a new function, which encourages safe usage of the test interface.
bpf_prog_test_run continues to work as before, but should be considered
unsafe.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/lib/bpf/bpf.c | 23 +++++++++++++++++++++++
 tools/lib/bpf/bpf.h | 19 +++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index ce1822194590..6e06484a7a99 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -463,6 +463,29 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	return ret;
 }
 
+int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
+{
+	union bpf_attr attr;
+	int ret;
+
+	if (!test_attr->data_out && test_attr->data_size_out > 0)
+		return -EINVAL;
+
+	bzero(&attr, sizeof(attr));
+	attr.test.prog_fd = test_attr->prog_fd;
+	attr.test.data_in = ptr_to_u64(test_attr->data_in);
+	attr.test.data_out = ptr_to_u64(test_attr->data_out);
+	attr.test.data_size_in = test_attr->data_size_in;
+	attr.test.data_size_out = test_attr->data_size_out;
+	attr.test.repeat = test_attr->repeat;
+
+	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+	test_attr->data_size_out = attr.test.data_size_out;
+	test_attr->retval = attr.test.retval;
+	test_attr->duration = attr.test.duration;
+	return ret;
+}
+
 int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
 {
 	union bpf_attr attr;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 09e8bbe111d4..ce6f07399d41 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -118,6 +118,25 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
 LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
 				enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+	int prog_fd;
+	int repeat;
+	const void *data_in;
+	__u32 data_size_in;
+	void *data_out;      /* optional */
+	__u32 data_size_out; /* in: max length of data_out
+			      * out: length of data_out */
+	__u32 retval;        /* out: return code of the BPF program */
+	__u32 duration;      /* out: average per repetition in ns */
+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);
+
+/*
+ * bpf_prog_test_run does not check that data_out is large enough. Consider
+ * using bpf_prog_test_run_xattr instead.
+ */
 LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
 				 __u32 size, void *data_out, __u32 *size_out,
 				 __u32 *retval, __u32 *duration);
-- 
2.17.1

[PATCH v4 4/4] selftests: add a test for bpf_prog_test_run_xattr

[Lorenz Bauer]

Make sure that bpf_prog_test_run_xattr returns the correct length
and that the kernel respects the output size hint. Also check
that errno indicates ENOSPC if there is a short output buffer given.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/test_progs.c | 55 +++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..ee827deb948a 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -70,7 +70,7 @@ static struct {
 	.tcp.urg_ptr = 123,
 };
 
-#define CHECK(condition, tag, format...) ({				\
+#define _CHECK(condition, tag, duration, format...) ({			\
 	int __ret = !!(condition);					\
 	if (__ret) {							\
 		error_cnt++;						\
@@ -83,6 +83,11 @@ static struct {
 	__ret;								\
 })
 
+#define CHECK(condition, tag, format...) \
+	_CHECK(condition, tag, duration, format)
+#define CHECK_ATTR(condition, tag, format...) \
+	_CHECK(condition, tag, tattr.duration, format)
+
 static int bpf_find_map(const char *test, struct bpf_object *obj,
 			const char *name)
 {
@@ -124,6 +129,53 @@ static void test_pkt_access(void)
 	bpf_object__close(obj);
 }
 
+static void test_prog_run_xattr(void)
+{
+	const char *file = "./test_pkt_access.o";
+	struct bpf_object *obj;
+	char buf[10];
+	int err;
+	struct bpf_prog_test_run_attr tattr = {
+		.repeat = 1,
+		.data_in = &pkt_v4,
+		.data_size_in = sizeof(pkt_v4),
+		.data_out = buf,
+		.data_size_out = 5,
+	};
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj,
+			    &tattr.prog_fd);
+	if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno))
+		return;
+
+	memset(buf, 0, sizeof(buf));
+
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err != -1 || errno != ENOSPC || tattr.retval, "run",
+	      "err %d errno %d retval %d\n", err, errno, tattr.retval);
+
+	CHECK_ATTR(tattr.data_size_out != sizeof(pkt_v4), "data_size_out",
+	      "incorrect output size, want %lu have %u\n",
+	      sizeof(pkt_v4), tattr.data_size_out);
+
+	CHECK_ATTR(buf[5] != 0, "overflow",
+	      "BPF_PROG_TEST_RUN ignored size hint\n");
+
+	tattr.data_out = NULL;
+	tattr.data_size_out = 0;
+	errno = 0;
+
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err || errno || tattr.retval, "run_no_output",
+	      "err %d errno %d retval %d\n", err, errno, tattr.retval);
+
+	tattr.data_size_out = 1;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err != -EINVAL, "run_wrong_size_out", "err %d\n", err);
+
+	bpf_object__close(obj);
+}
+
 static void test_xdp(void)
 {
 	struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1837,6 +1889,7 @@ int main(void)
 	jit_enabled = is_jit_enabled();
 
 	test_pkt_access();
+	test_prog_run_xattr();
 	test_xdp();
 	test_xdp_adjust_tail();
 	test_l4lb_all();
-- 
2.17.1

Re: [PATCH v4 3/4] libbpf: add bpf_prog_test_run_xattr

[Alexei Starovoitov]

On Wed, Nov 28, 2018 at 04:53:11PM +0000, Lorenz Bauer wrote:
> Add a new function, which encourages safe usage of the test interface.
> bpf_prog_test_run continues to work as before, but should be considered
> unsafe.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
..
> +
> +LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);

it fails to be build due to:
  LINK     tools/testing/selftests/bpf/test_libbpf
(117) does NOT match with num of versioned symbols in testing/selftests/bpf/libbpf.so (116).
Please make sure all LIBBPF_API symbols are versioned in libbpf.map.

Please fixup and respin.
The rest looks good.

[PATCH v5 0/4] Fix unsafe BPF_PROG_TEST_RUN interface

[Lorenz Bauer]

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.

Thanks to everyone for their advice and patience with this patch set!

Changes in v5:
* Fix up libbpf.map

Changes in v4:
* Document bpf_prog_test_run and bpf_prog_test_run_xattr
* Use struct bpf_prog_test_run_attr for return values

Changes in v3:
* Introduce bpf_prog_test_run_xattr instead of modifying the existing
  function

Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out

Lorenz Bauer (4):
  bpf: respect size hint to BPF_PROG_TEST_RUN if present
  tools: sync uapi/linux/bpf.h
  libbpf: add bpf_prog_test_run_xattr
  selftests: add a test for bpf_prog_test_run_xattr

 include/uapi/linux/bpf.h                 |  7 ++-
 net/bpf/test_run.c                       | 15 ++++++-
 tools/include/uapi/linux/bpf.h           |  7 ++-
 tools/lib/bpf/bpf.c                      | 23 ++++++++++
 tools/lib/bpf/bpf.h                      | 19 ++++++++
 tools/lib/bpf/libbpf.map                 |  1 +
 tools/testing/selftests/bpf/test_progs.c | 55 +++++++++++++++++++++++-
 7 files changed, 120 insertions(+), 7 deletions(-)

-- 
2.19.1

[PATCH v5 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present

[Lorenz Bauer]

Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/uapi/linux/bpf.h |  7 +++++--
 net/bpf/test_run.c       | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 {
 	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
 	int err = -EFAULT;
+	u32 copy_size = size;
 
-	if (data_out && copy_to_user(data_out, data, size))
+	/* Clamp copy if the user has provided a size hint, but copy the full
+	 * buffer if not to retain old behaviour.
+	 */
+	if (kattr->test.data_size_out &&
+	    copy_size > kattr->test.data_size_out) {
+		copy_size = kattr->test.data_size_out;
+		err = -ENOSPC;
+	}
+
+	if (data_out && copy_to_user(data_out, data, copy_size))
 		goto out;
 	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
 		goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 		goto out;
 	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
 		goto out;
-	err = 0;
+	if (err != -ENOSPC)
+		err = 0;
 out:
 	return err;
 }
-- 
2.19.1

[PATCH v5 2/4] tools: sync uapi/linux/bpf.h

[Lorenz Bauer]

Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/include/uapi/linux/bpf.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
 		__u32		prog_fd;
 		__u32		retval;
-		__u32		data_size_in;
-		__u32		data_size_out;
+		__u32		data_size_in;	/* input: len of data_in */
+		__u32		data_size_out;	/* input/output: len of data_out
+						 *   returns ENOSPC if data_out
+						 *   is too small.
+						 */
 		__aligned_u64	data_in;
 		__aligned_u64	data_out;
 		__u32		repeat;
-- 
2.19.1

[PATCH v5 3/4] libbpf: add bpf_prog_test_run_xattr

[Lorenz Bauer]

Add a new function, which encourages safe usage of the test interface.
bpf_prog_test_run continues to work as before, but should be considered
unsafe.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/lib/bpf/bpf.c      | 23 +++++++++++++++++++++++
 tools/lib/bpf/bpf.h      | 19 +++++++++++++++++++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 43 insertions(+)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index ce1822194590..6e06484a7a99 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -463,6 +463,29 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	return ret;
 }
 
+int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
+{
+	union bpf_attr attr;
+	int ret;
+
+	if (!test_attr->data_out && test_attr->data_size_out > 0)
+		return -EINVAL;
+
+	bzero(&attr, sizeof(attr));
+	attr.test.prog_fd = test_attr->prog_fd;
+	attr.test.data_in = ptr_to_u64(test_attr->data_in);
+	attr.test.data_out = ptr_to_u64(test_attr->data_out);
+	attr.test.data_size_in = test_attr->data_size_in;
+	attr.test.data_size_out = test_attr->data_size_out;
+	attr.test.repeat = test_attr->repeat;
+
+	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+	test_attr->data_size_out = attr.test.data_size_out;
+	test_attr->retval = attr.test.retval;
+	test_attr->duration = attr.test.duration;
+	return ret;
+}
+
 int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
 {
 	union bpf_attr attr;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 09e8bbe111d4..ce6f07399d41 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -118,6 +118,25 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
 LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
 				enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+	int prog_fd;
+	int repeat;
+	const void *data_in;
+	__u32 data_size_in;
+	void *data_out;      /* optional */
+	__u32 data_size_out; /* in: max length of data_out
+			      * out: length of data_out */
+	__u32 retval;        /* out: return code of the BPF program */
+	__u32 duration;      /* out: average per repetition in ns */
+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);
+
+/*
+ * bpf_prog_test_run does not check that data_out is large enough. Consider
+ * using bpf_prog_test_run_xattr instead.
+ */
 LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
 				 __u32 size, void *data_out, __u32 *size_out,
 				 __u32 *retval, __u32 *duration);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 4fb29f6d7a80..8deff22d61bb 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -65,6 +65,7 @@ LIBBPF_0.0.1 {
 		bpf_prog_load_xattr;
 		bpf_prog_query;
 		bpf_prog_test_run;
+		bpf_prog_test_run_xattr;
 		bpf_program__fd;
 		bpf_program__is_kprobe;
 		bpf_program__is_perf_event;
-- 
2.19.1

[PATCH v5 4/4] selftests: add a test for bpf_prog_test_run_xattr

[Lorenz Bauer]

Make sure that bpf_prog_test_run_xattr returns the correct length
and that the kernel respects the output size hint. Also check
that errno indicates ENOSPC if there is a short output buffer given.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/test_progs.c | 55 +++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..ee827deb948a 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -70,7 +70,7 @@ static struct {
 	.tcp.urg_ptr = 123,
 };
 
-#define CHECK(condition, tag, format...) ({				\
+#define _CHECK(condition, tag, duration, format...) ({			\
 	int __ret = !!(condition);					\
 	if (__ret) {							\
 		error_cnt++;						\
@@ -83,6 +83,11 @@ static struct {
 	__ret;								\
 })
 
+#define CHECK(condition, tag, format...) \
+	_CHECK(condition, tag, duration, format)
+#define CHECK_ATTR(condition, tag, format...) \
+	_CHECK(condition, tag, tattr.duration, format)
+
 static int bpf_find_map(const char *test, struct bpf_object *obj,
 			const char *name)
 {
@@ -124,6 +129,53 @@ static void test_pkt_access(void)
 	bpf_object__close(obj);
 }
 
+static void test_prog_run_xattr(void)
+{
+	const char *file = "./test_pkt_access.o";
+	struct bpf_object *obj;
+	char buf[10];
+	int err;
+	struct bpf_prog_test_run_attr tattr = {
+		.repeat = 1,
+		.data_in = &pkt_v4,
+		.data_size_in = sizeof(pkt_v4),
+		.data_out = buf,
+		.data_size_out = 5,
+	};
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj,
+			    &tattr.prog_fd);
+	if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno))
+		return;
+
+	memset(buf, 0, sizeof(buf));
+
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err != -1 || errno != ENOSPC || tattr.retval, "run",
+	      "err %d errno %d retval %d\n", err, errno, tattr.retval);
+
+	CHECK_ATTR(tattr.data_size_out != sizeof(pkt_v4), "data_size_out",
+	      "incorrect output size, want %lu have %u\n",
+	      sizeof(pkt_v4), tattr.data_size_out);
+
+	CHECK_ATTR(buf[5] != 0, "overflow",
+	      "BPF_PROG_TEST_RUN ignored size hint\n");
+
+	tattr.data_out = NULL;
+	tattr.data_size_out = 0;
+	errno = 0;
+
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err || errno || tattr.retval, "run_no_output",
+	      "err %d errno %d retval %d\n", err, errno, tattr.retval);
+
+	tattr.data_size_out = 1;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err != -EINVAL, "run_wrong_size_out", "err %d\n", err);
+
+	bpf_object__close(obj);
+}
+
 static void test_xdp(void)
 {
 	struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1837,6 +1889,7 @@ int main(void)
 	jit_enabled = is_jit_enabled();
 
 	test_pkt_access();
+	test_prog_run_xattr();
 	test_xdp();
 	test_xdp_adjust_tail();
 	test_l4lb_all();
-- 
2.19.1

Re: [PATCH v5 0/4] Fix unsafe BPF_PROG_TEST_RUN interface

[Alexei Starovoitov]

On Mon, Dec 03, 2018 at 11:31:22AM +0000, Lorenz Bauer wrote:
> Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
> This is because bpf_test_finish copies the output buffer to user space
> without checking its size. This can lead to the kernel overwriting
> data in user space after the buffer if xdp_adjust_head and friends are
> in play.
> 
> Thanks to everyone for their advice and patience with this patch set!
> 
> Changes in v5:
> * Fix up libbpf.map
> 
> Changes in v4:
> * Document bpf_prog_test_run and bpf_prog_test_run_xattr
> * Use struct bpf_prog_test_run_attr for return values
> 
> Changes in v3:
> * Introduce bpf_prog_test_run_xattr instead of modifying the existing
>   function
> 
> Changes in v2:
> * Make the syscall return ENOSPC if data_size_out is too small
> * Make bpf_prog_test_run return EINVAL if size_out is missing
> * Document the new behaviour of data_size_out

Applied, Thanks