[LTP] [PATCH 1/3] bpf: Print full verification log

[LTP] [PATCH 1/3] bpf: Print full verification log

[Richard Palethorpe]

The log never falls within the 1024 byte limit imposed by format
string buffer. So print it separately with dprintf.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/kernel/syscalls/bpf/bpf_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index ba0829a75..6e9cd498c 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2019-2020 Linux Test Project
  */
 
+#include <stdio.h>
+
 #define TST_NO_DEFAULT_MAIN
 #include "tst_test.h"
 #include "bpf_common.h"
@@ -118,8 +120,10 @@ int bpf_load_prog(union bpf_attr *const attr, const char *const log)
 	if (ret != -1)
 		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
 
-	if (log[0] != 0)
-		tst_brk(TBROK | TERRNO, "Failed verification: %s", log);
+	if (log[0] != 0) {
+		dprintf(STDERR_FILENO, "%s\n", log);
+		tst_brk(TBROK | TERRNO, "Failed verification");
+	}
 
 	tst_brk(TBROK | TERRNO, "Failed to load program");
 	return ret;
-- 
2.31.1

[LTP] [PATCH 2/3] bpf: Mention CAP_BPF in required privs and add fallback definition

[Richard Palethorpe]

We don't need CAP_SYS_ADMIN most of the time. bpf() can be called with
only CAP_BPF even when unprivileged_bpf_disable > 0. When
unprivileged_bpf_disable == 0, CAP_BPF also allows more features Vs. no
privileges at all.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/capability.h                  | 4 ++++
 testcases/kernel/syscalls/bpf/bpf_common.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/lapi/capability.h b/include/lapi/capability.h
index c6470f389..8cabd0f28 100644
--- a/include/lapi/capability.h
+++ b/include/lapi/capability.h
@@ -44,6 +44,10 @@
 # define CAP_SYS_RESOURCE     24
 #endif
 
+#ifndef CAP_BPF
+# define CAP_BPF              39
+#endif
+
 #ifndef CAP_TO_INDEX
 # define CAP_TO_INDEX(x)     ((x) >> 5)
 #endif
diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index 6e9cd498c..72957a487 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -40,7 +40,7 @@ int bpf_map_create(union bpf_attr *const attr)
 		if (errno == EPERM) {
 			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
 			tst_brk(TCONF | TERRNO,
-				"bpf() requires CAP_SYS_ADMIN on this system");
+				"bpf() requires CAP_SYS_ADMIN or CAP_BPF on this system");
 		} else {
 			tst_brk(TBROK | TERRNO, "Failed to create array map");
 		}
-- 
2.31.1

[LTP] [PATCH 3/3] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Richard Palethorpe]

On older kernels pointer arithmetic requires CAP_BPF. They also lack
the ability to call BPF subprogs. This makes it difficult to exploit
the div/mod behavior.

Older kernels leave div/mod by zero undefined. This causes the test to
fail and backporting the new behavior is difficult. So when we find
that pointer arithmetic is not possible without CAP_BPF we can return
TCONF. Because in this case, we know the test will fail, the risk is
limited and there is little that can be done about it.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/bpf.h                         |  1 +
 testcases/kernel/syscalls/bpf/bpf_prog05.c | 44 +++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
index 0e4527b8b..f9e50c6b7 100644
--- a/include/lapi/bpf.h
+++ b/include/lapi/bpf.h
@@ -27,6 +27,7 @@
 #define BPF_JNE		0x50	/* jump != */
 
 #define BPF_SIZE(code)  ((code) & 0x18)
+#define		BPF_B		0x10 /*  8-bit */
 #define		BPF_W		0x00    /* 32-bit */
 #define         BPF_DW		0x18	/* double word (64-bit) */
 
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog05.c b/testcases/kernel/syscalls/bpf/bpf_prog05.c
index b2792c505..2be5a2cc9 100644
--- a/testcases/kernel/syscalls/bpf/bpf_prog05.c
+++ b/testcases/kernel/syscalls/bpf/bpf_prog05.c
@@ -63,6 +63,45 @@ static uint64_t *val;
 static char *log;
 static union bpf_attr *attr;
 
+static void ensure_ptr_arithmetic(void)
+{
+	const struct bpf_insn prog_insn[] = {
+		/* r2 = r10
+		 * r3 = -1
+		 * r2 += r3
+		 * *(char *)r2 = 0
+		 */
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+		BPF_MOV64_IMM(BPF_REG_3, -1),
+		BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
+		BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0),
+
+		/* exit(0) */
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN()
+	};
+	int ret;
+
+	bpf_init_prog_attr(attr, prog_insn, sizeof(prog_insn), log, BUFSIZE);
+
+	ret = TST_RETRY_FUNC(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)),
+				       TST_RETVAL_GE0);
+
+	if (ret >= 0) {
+		tst_res(TINFO, "Have pointer arithmetic");
+		SAFE_CLOSE(ret);
+		return;
+	}
+
+	if (ret != -1)
+		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
+
+	if (log[0] != 0)
+		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
+
+	tst_brk(TBROK | TERRNO, "Failed to load program");
+}
+
 static int load_prog(void)
 {
 	const struct bpf_insn prog_insn[] = {
@@ -132,7 +171,9 @@ static void run(void)
 {
 	int prog_fd;
 
-	map_fd = bpf_map_array_create(4);
+	map_fd = bpf_map_array_create(8);
+
+	ensure_ptr_arithmetic();
 	prog_fd = load_prog();
 	bpf_run_prog(prog_fd, msg, sizeof(MSG));
 	SAFE_CLOSE(prog_fd);
@@ -157,6 +198,7 @@ static struct tst_test test = {
 	.taint_check = TST_TAINT_W | TST_TAINT_D,
 	.caps = (struct tst_cap []) {
 		TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
+		TST_CAP(TST_CAP_DROP, CAP_BPF),
 		{}
 	},
 	.bufs = (struct tst_buffers []) {
-- 
2.31.1

[LTP] [PATCH 1/3] bpf: Print full verification log

[Cyril Hrubis]

Hi!
> The log never falls within the 1024 byte limit imposed by format
> string buffer. So print it separately with dprintf.

I guess that it would be slightly cleaner to add tst_printf() which
would print the output so that we don't have to hardcode printing data
into STDERR in the test like this.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/4] API: Add tst_printf to avoid specifying the output FD in tests

[Richard Palethorpe]

In bpf_common.h we have to print the verifier log with
dprintf(STDERR_FILENO, ...)  because it is usually too large for
tst_{res,brk}(). As these functions use sprintf() and write() to allow
printing in signal handlers.

We can hide the STDERR_FILENO part in the library. Just incase we want
to change the fileno at some point.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V2: Add this patch and use tst_printf

 include/tst_test.h | 3 +++
 lib/tst_test.c     | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/tst_test.h b/include/tst_test.h
index 27ebed94e..5e3619698 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -79,6 +79,9 @@ void tst_brk_(const char *file, const int lineno, int ttype,
 		tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
 	})
 
+void tst_printf(const char *const fmt, ...)
+		__attribute__((nonnull(1), format (printf, 1, 2)));
+
 /* flush stderr and stdout */
 void tst_flush(void);
 
diff --git a/lib/tst_test.c b/lib/tst_test.c
index b61aa8b03..4224353da 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -360,6 +360,15 @@ void tst_brk_(const char *file, const int lineno, int ttype,
 	va_end(va);
 }
 
+void tst_printf(const char *const fmt, ...)
+{
+	va_list va;
+
+	va_start(va, fmt);
+	vdprintf(STDERR_FILENO, fmt, va);
+	va_end(va);
+}
+
 static void check_child_status(pid_t pid, int status)
 {
 	int ret;
-- 
2.31.1

[LTP] [PATCH v2 2/4] bpf: Print full verification log

[Richard Palethorpe]

The log never falls within the 1024 byte limit imposed by format
string buffer. So print it separately with dprintf.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/kernel/syscalls/bpf/bpf_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index ba0829a75..aac235cac 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2019-2020 Linux Test Project
  */
 
+#include <stdio.h>
+
 #define TST_NO_DEFAULT_MAIN
 #include "tst_test.h"
 #include "bpf_common.h"
@@ -118,8 +120,10 @@ int bpf_load_prog(union bpf_attr *const attr, const char *const log)
 	if (ret != -1)
 		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
 
-	if (log[0] != 0)
-		tst_brk(TBROK | TERRNO, "Failed verification: %s", log);
+	if (log[0] != 0) {
+		tst_printf("%s\n", log);
+		tst_brk(TBROK | TERRNO, "Failed verification");
+	}
 
 	tst_brk(TBROK | TERRNO, "Failed to load program");
 	return ret;
-- 
2.31.1

[LTP] [PATCH v2 2/4] bpf: Print full verification log

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> index ba0829a75..aac235cac 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_common.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> @@ -3,6 +3,8 @@
>   * Copyright (c) 2019-2020 Linux Test Project
>   */
>  
> +#include <stdio.h>

I've removed this now unused include.

>  #define TST_NO_DEFAULT_MAIN
>  #include "tst_test.h"
>  #include "bpf_common.h"
> @@ -118,8 +120,10 @@ int bpf_load_prog(union bpf_attr *const attr, const char *const log)
>  	if (ret != -1)
>  		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
>  
> -	if (log[0] != 0)
> -		tst_brk(TBROK | TERRNO, "Failed verification: %s", log);
> +	if (log[0] != 0) {
> +		tst_printf("%s\n", log);
> +		tst_brk(TBROK | TERRNO, "Failed verification");
> +	}
>  
>  	tst_brk(TBROK | TERRNO, "Failed to load program");
>  	return ret;

And pushed the whole patchset, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 3/4] bpf: Mention CAP_BPF in required privs and add fallback definition

[Richard Palethorpe]

We don't need CAP_SYS_ADMIN most of the time. bpf() can be called with
only CAP_BPF even when unprivileged_bpf_disable > 0. When
unprivileged_bpf_disable == 0, CAP_BPF also allows more features Vs. no
privileges at all.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/capability.h                  | 4 ++++
 testcases/kernel/syscalls/bpf/bpf_common.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/lapi/capability.h b/include/lapi/capability.h
index c6470f389..8cabd0f28 100644
--- a/include/lapi/capability.h
+++ b/include/lapi/capability.h
@@ -44,6 +44,10 @@
 # define CAP_SYS_RESOURCE     24
 #endif
 
+#ifndef CAP_BPF
+# define CAP_BPF              39
+#endif
+
 #ifndef CAP_TO_INDEX
 # define CAP_TO_INDEX(x)     ((x) >> 5)
 #endif
diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index aac235cac..3afa6c51e 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -40,7 +40,7 @@ int bpf_map_create(union bpf_attr *const attr)
 		if (errno == EPERM) {
 			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
 			tst_brk(TCONF | TERRNO,
-				"bpf() requires CAP_SYS_ADMIN on this system");
+				"bpf() requires CAP_SYS_ADMIN or CAP_BPF on this system");
 		} else {
 			tst_brk(TBROK | TERRNO, "Failed to create array map");
 		}
-- 
2.31.1

[LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Richard Palethorpe]

On older kernels pointer arithmetic requires CAP_BPF. They also lack
the ability to call BPF subprogs. This makes it difficult to exploit
the div/mod behavior.

Older kernels leave div/mod by zero undefined. This causes the test to
fail and backporting the new behavior is difficult. So when we find
that pointer arithmetic is not possible without CAP_BPF we can return
TCONF. Because in this case, we know the test will fail, the risk is
limited and there is little that can be done about it.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/bpf.h                         |  1 +
 testcases/kernel/syscalls/bpf/bpf_prog05.c | 44 +++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
index 0e4527b8b..f9e50c6b7 100644
--- a/include/lapi/bpf.h
+++ b/include/lapi/bpf.h
@@ -27,6 +27,7 @@
 #define BPF_JNE		0x50	/* jump != */
 
 #define BPF_SIZE(code)  ((code) & 0x18)
+#define		BPF_B		0x10 /*  8-bit */
 #define		BPF_W		0x00    /* 32-bit */
 #define         BPF_DW		0x18	/* double word (64-bit) */
 
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog05.c b/testcases/kernel/syscalls/bpf/bpf_prog05.c
index b2792c505..2be5a2cc9 100644
--- a/testcases/kernel/syscalls/bpf/bpf_prog05.c
+++ b/testcases/kernel/syscalls/bpf/bpf_prog05.c
@@ -63,6 +63,45 @@ static uint64_t *val;
 static char *log;
 static union bpf_attr *attr;
 
+static void ensure_ptr_arithmetic(void)
+{
+	const struct bpf_insn prog_insn[] = {
+		/* r2 = r10
+		 * r3 = -1
+		 * r2 += r3
+		 * *(char *)r2 = 0
+		 */
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+		BPF_MOV64_IMM(BPF_REG_3, -1),
+		BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
+		BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0),
+
+		/* exit(0) */
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN()
+	};
+	int ret;
+
+	bpf_init_prog_attr(attr, prog_insn, sizeof(prog_insn), log, BUFSIZE);
+
+	ret = TST_RETRY_FUNC(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)),
+				       TST_RETVAL_GE0);
+
+	if (ret >= 0) {
+		tst_res(TINFO, "Have pointer arithmetic");
+		SAFE_CLOSE(ret);
+		return;
+	}
+
+	if (ret != -1)
+		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
+
+	if (log[0] != 0)
+		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
+
+	tst_brk(TBROK | TERRNO, "Failed to load program");
+}
+
 static int load_prog(void)
 {
 	const struct bpf_insn prog_insn[] = {
@@ -132,7 +171,9 @@ static void run(void)
 {
 	int prog_fd;
 
-	map_fd = bpf_map_array_create(4);
+	map_fd = bpf_map_array_create(8);
+
+	ensure_ptr_arithmetic();
 	prog_fd = load_prog();
 	bpf_run_prog(prog_fd, msg, sizeof(MSG));
 	SAFE_CLOSE(prog_fd);
@@ -157,6 +198,7 @@ static struct tst_test test = {
 	.taint_check = TST_TAINT_W | TST_TAINT_D,
 	.caps = (struct tst_cap []) {
 		TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
+		TST_CAP(TST_CAP_DROP, CAP_BPF),
 		{}
 	},
 	.bufs = (struct tst_buffers []) {
-- 
2.31.1

Re: [LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Joerg Vehlow]

Hi Richard,

Am 8/31/2021 um 11:10 AM schrieb Richard Palethorpe via ltp:
> On older kernels pointer arithmetic requires CAP_BPF. They also lack
> the ability to call BPF subprogs. This makes it difficult to exploit
> the div/mod behavior.
> 
> Older kernels leave div/mod by zero undefined. This causes the test to
> fail and backporting the new behavior is difficult. So when we find
> that pointer arithmetic is not possible without CAP_BPF we can return
> TCONF. Because in this case, we know the test will fail, the risk is
> limited and there is little that can be done about it.

What does older kernel mean here?

> +
> +	if (ret != -1)
> +		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
> +
> +	if (log[0] != 0)
> +		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
This check now fails for me with the following output, where the test 
was successful, before this patch. The kernel is a non-standard suse 
5.3.18 with realtime patches.

bpf_prog05.c:100: TCONF: No pointer arithmetic:
  0: (bf) r2 = r10
1: (b7) r3 = -1
2: (0f) r2 += r3
3: (72) *(u8 *)(r2 +0) = 0
4: (b7) r0 = 0
5: (95) exit

from 2 to 3 (speculative execution): R1=ctx(id=0,off=0,imm=0) R2_w=fp0 
R3_w=invP-1 R10=fp0
3: (72) *(u8 *)(r2 +0) = 0
invalid stack off=0 size=1
processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0 
peak_states 0 mark_read 0
: EACCES (13)


Is this too old? But then again, the test was successful before this 
patch and your commit message states, that it was not successful on 
"older kernels".

Joerg

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Richard Palethorpe]

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi Richard,
>
> Am 8/31/2021 um 11:10 AM schrieb Richard Palethorpe via ltp:
>> On older kernels pointer arithmetic requires CAP_BPF. They also lack
>> the ability to call BPF subprogs. This makes it difficult to exploit
>> the div/mod behavior.
>> Older kernels leave div/mod by zero undefined. This causes the test
>> to
>> fail and backporting the new behavior is difficult. So when we find
>> that pointer arithmetic is not possible without CAP_BPF we can return
>> TCONF. Because in this case, we know the test will fail, the risk is
>> limited and there is little that can be done about it.
>
> What does older kernel mean here?

I'm refering to multiple different kernel versions (perhaps
confusingly). On much older kernels we don't have any issues because
pointer arithmetic was not possible in sensitive contexts.

Then it was made possible, but div/mod by zero was undefined. Then that
was fixed, but IIRC there were other issues. Depending on what commits
you have, any number of outcomes are possible.

>
>> +
>> +	if (ret != -1)
>> +		tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
>> +
>> +	if (log[0] != 0)
>> +		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
> This check now fails for me with the following output, where the test
> was successful, before this patch. The kernel is a non-standard suse 
> 5.3.18 with realtime patches.
>
> bpf_prog05.c:100: TCONF: No pointer arithmetic:
>  0: (bf) r2 = r10
> 1: (b7) r3 = -1
> 2: (0f) r2 += r3
> 3: (72) *(u8 *)(r2 +0) = 0
> 4: (b7) r0 = 0
> 5: (95) exit
>
> from 2 to 3 (speculative execution): R1=ctx(id=0,off=0,imm=0) R2_w=fp0
> R3_w=invP-1 R10=fp0
> 3: (72) *(u8 *)(r2 +0) = 0
> invalid stack off=0 size=1
> processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> : EACCES (13)
>
>
> Is this too old? But then again, the test was successful before this
> patch and your commit message states, that it was not successful on 
> "older kernels".

Are you testing on ARM? eBPF adopted the ARM behavior when dividing by
zero.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Joerg Vehlow]

Hi Richard,

Am 1/11/2022 um 3:36 PM schrieb Richard Palethorpe:
>>> +	if (log[0] != 0)
>>> +		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
>> This check now fails for me with the following output, where the test
>> was successful, before this patch. The kernel is a non-standard suse
>> 5.3.18 with realtime patches.
>>
>> bpf_prog05.c:100: TCONF: No pointer arithmetic:
>>   0: (bf) r2 = r10
>> 1: (b7) r3 = -1
>> 2: (0f) r2 += r3
>> 3: (72) *(u8 *)(r2 +0) = 0
>> 4: (b7) r0 = 0
>> 5: (95) exit
>>
>> from 2 to 3 (speculative execution): R1=ctx(id=0,off=0,imm=0) R2_w=fp0
>> R3_w=invP-1 R10=fp0
>> 3: (72) *(u8 *)(r2 +0) = 0
>> invalid stack off=0 size=1
>> processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0
>> peak_states 0 mark_read 0
>> : EACCES (13)
>>
>>
>> Is this too old? But then again, the test was successful before this
>> patch and your commit message states, that it was not successful on
>> "older kernels".
> 
> Are you testing on ARM? eBPF adopted the ARM behavior when dividing by
> zero.
> 

No this is x86 and I was expecting 5.3 to not be "very old". IIRC 
patches for the bug tested here are partly already integrated into 4.19, 
so I guess 5.3 shouldn't be "very old" in that context?

I am not sure what you tried to test, I guess accessing the stack using 
indirect pointer arithmetic? Because changing the store to "*(u8 *)(r10 
- 1) = 0", works. But if this indirection is required for the actually 
test, it should fail, but it doesn't -> I think the check tests 
something, that is not actually required for the actual test.
I guess what you wanted to check is what BPF_MAP_ARRAY_STX does. There 
is one major difference between your implementation of the check and 
BPF_MAP_ARRAY_STX: BPF_MAP_ARRAY_STX uses an immediate value, to add to 
r2 compared to a register in your check:

  * r2 = fp
  * r2 += r2 - 4

vs

  * r2 = r10
  * r3 = -1
  * r2 += r3

That is actually what makes the difference here...

If I modify the check like this it works:

--- a/testcases/kernel/syscalls/bpf/bpf_prog05.c
+++ b/testcases/kernel/syscalls/bpf/bpf_prog05.c
@@ -67,13 +67,11 @@ static void ensure_ptr_arithmetic(void)
  {
         const struct bpf_insn prog_insn[] = {
                 /* r2 = r10
-                * r3 = -1
-                * r2 += r3
+                * r2 += -1
                  * *(char *)r2 = 0
                  */
                 BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
-               BPF_MOV64_IMM(BPF_REG_3, -1),
-               BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
+               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -1),
                 BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0),

                 /* exit(0) */


Is this still checking what it is supposed to? Then I would send it post 
this as a patch

Joerg

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Richard Palethorpe]

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi Richard,
>
> Am 1/11/2022 um 3:36 PM schrieb Richard Palethorpe:
>>>> +	if (log[0] != 0)
>>>> +		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
>>> This check now fails for me with the following output, where the test
>>> was successful, before this patch. The kernel is a non-standard suse
>>> 5.3.18 with realtime patches.
>>>
>>> bpf_prog05.c:100: TCONF: No pointer arithmetic:
>>>   0: (bf) r2 = r10
>>> 1: (b7) r3 = -1
>>> 2: (0f) r2 += r3
>>> 3: (72) *(u8 *)(r2 +0) = 0
>>> 4: (b7) r0 = 0
>>> 5: (95) exit
>>>
>>> from 2 to 3 (speculative execution): R1=ctx(id=0,off=0,imm=0) R2_w=fp0
>>> R3_w=invP-1 R10=fp0
>>> 3: (72) *(u8 *)(r2 +0) = 0
>>> invalid stack off=0 size=1
>>> processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0
>>> peak_states 0 mark_read 0
>>> : EACCES (13)
>>>
>>>
>>> Is this too old? But then again, the test was successful before this
>>> patch and your commit message states, that it was not successful on
>>> "older kernels".
>> Are you testing on ARM? eBPF adopted the ARM behavior when dividing
>> by
>> zero.
>> 
>
> No this is x86 and I was expecting 5.3 to not be "very old". IIRC
> patches for the bug tested here are partly already integrated into
> 4.19, so I guess 5.3 shouldn't be "very old" in that context?
>
> I am not sure what you tried to test, I guess accessing the stack
> using indirect pointer arithmetic? Because changing the store to "*(u8
> *)(r10 - 1) = 0", works. But if this indirection is required for the
> actually test, it should fail, but it doesn't -> I think the check
> tests something, that is not actually required for the actual test.

The actual test just checks what the result of modulo and division by
zero are against the 'arbitrary' values chosen by ARM/upstream (simlar
to the selftests BTW). This is because there are multiple different
values, which can be exploited depending on what patches are applied.

However they require different logic to detect if pointer arithmetic
resulted in an OOB read or write. Note that even if you can do pointer
arithmetic and it results in the wrong value being accessed. It doesn't
matter if it's not OOB or if the validator blocks it because it has
calculated the offset in the wrong direction. It may still be
exploitable in these cases, but the test will pass.

I can't remember what the exact problems were; it got very complicated
so I just checked that the results of division/modulo by zero matched
upstream. This resulted in a false positive on some kernel which in
theory would be vulnerable if unprivileged pointer arithmetic were
possible. So I added the ptr arithmetic check to prevent the test from
running on kernels like this.

This also appears to prevent the test from running on your kernel which
is not vulnerable in any case. Thinking about it, this may be due to a
configuration option you have set
(i.e. sys/kernel/unprivileged_bpf_disabled). Perhaps the problem is this
is not mentioned in the test description or TCONF message?

IIRC We chose TCONF instead of TPASS to give a hint that we're not
running the full test. You could also have a vulnerable kernel, but with
unpriveleged BPF disabled by a config option. In this case the test
should not return TCONF because a user could change the option and make
themselves vulnerable.

> I guess what you wanted to check is what BPF_MAP_ARRAY_STX does. There
> is one major difference between your implementation of the check and 
> BPF_MAP_ARRAY_STX: BPF_MAP_ARRAY_STX uses an immediate value, to add
> to r2 compared to a register in your check:
>
>  * r2 = fp
>  * r2 += r2 - 4
>
> vs
>
>  * r2 = r10
>  * r3 = -1
>  * r2 += r3
>
> That is actually what makes the difference here...

No, BPF_MAP_ARRAY_STX is just used to output the result of div/mod by
zero and I don't think it needs testing.

>
> If I modify the check like this it works:
>
> --- a/testcases/kernel/syscalls/bpf/bpf_prog05.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog05.c
> @@ -67,13 +67,11 @@ static void ensure_ptr_arithmetic(void)
>  {
>         const struct bpf_insn prog_insn[] = {
>                 /* r2 = r10
> -                * r3 = -1
> -                * r2 += r3
> +                * r2 += -1
>                  * *(char *)r2 = 0
>                  */
>                 BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> -               BPF_MOV64_IMM(BPF_REG_3, -1),
> -               BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
> +               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -1),
>                 BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0),
>
>                 /* exit(0) */
>
>
> Is this still checking what it is supposed to? Then I would send it
> post this as a patch
>
> Joerg

This will reintroduce the false positive.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Joerg Vehlow]

Hi Richard,

Am 1/13/2022 um 8:48 AM schrieb Richard Palethorpe:
> Hello Joerg,
> 
> Joerg Vehlow <lkml@jv-coder.de> writes:
> 
>> Hi Richard,
>>
>> Am 1/11/2022 um 3:36 PM schrieb Richard Palethorpe:
>>>>> +	if (log[0] != 0)
>>>>> +		tst_brk(TCONF | TERRNO, "No pointer arithmetic:\n %s", log);
>>>> This check now fails for me with the following output, where the test
>>>> was successful, before this patch. The kernel is a non-standard suse
>>>> 5.3.18 with realtime patches.
>>>>
>>>> bpf_prog05.c:100: TCONF: No pointer arithmetic:
>>>>   0: (bf) r2 = r10
>>>> 1: (b7) r3 = -1
>>>> 2: (0f) r2 += r3
>>>> 3: (72) *(u8 *)(r2 +0) = 0
>>>> 4: (b7) r0 = 0
>>>> 5: (95) exit
>>>>
>>>> from 2 to 3 (speculative execution): R1=ctx(id=0,off=0,imm=0) R2_w=fp0
>>>> R3_w=invP-1 R10=fp0
>>>> 3: (72) *(u8 *)(r2 +0) = 0
>>>> invalid stack off=0 size=1
>>>> processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0
>>>> peak_states 0 mark_read 0
>>>> : EACCES (13)
>>>>
>>>>
>>>> Is this too old? But then again, the test was successful before this
>>>> patch and your commit message states, that it was not successful on
>>>> "older kernels".
>>> Are you testing on ARM? eBPF adopted the ARM behavior when dividing
>>> by
>>> zero.
>>>
>>
>> No this is x86 and I was expecting 5.3 to not be "very old". IIRC
>> patches for the bug tested here are partly already integrated into
>> 4.19, so I guess 5.3 shouldn't be "very old" in that context?
>>
>> I am not sure what you tried to test, I guess accessing the stack
>> using indirect pointer arithmetic? Because changing the store to "*(u8
>> *)(r10 - 1) = 0", works. But if this indirection is required for the
>> actually test, it should fail, but it doesn't -> I think the check
>> tests something, that is not actually required for the actual test.
> 
> The actual test just checks what the result of modulo and division by
> zero are against the 'arbitrary' values chosen by ARM/upstream (simlar
> to the selftests BTW). This is because there are multiple different
> values, which can be exploited depending on what patches are applied.
> 
> However they require different logic to detect if pointer arithmetic
> resulted in an OOB read or write. Note that even if you can do pointer
> arithmetic and it results in the wrong value being accessed. It doesn't
> matter if it's not OOB or if the validator blocks it because it has
> calculated the offset in the wrong direction. It may still be
> exploitable in these cases, but the test will pass.
> 
> I can't remember what the exact problems were; it got very complicated
> so I just checked that the results of division/modulo by zero matched
> upstream. This resulted in a false positive on some kernel which in
> theory would be vulnerable if unprivileged pointer arithmetic were
> possible. So I added the ptr arithmetic check to prevent the test from
> running on kernels like this.
> 
> This also appears to prevent the test from running on your kernel which
> is not vulnerable in any case. Thinking about it, this may be due to a
> configuration option you have set
> (i.e. sys/kernel/unprivileged_bpf_disabled). Perhaps the problem is this
> is not mentioned in the test description or TCONF message?
> 
> IIRC We chose TCONF instead of TPASS to give a hint that we're not
> running the full test. You could also have a vulnerable kernel, but with
> unpriveleged BPF disabled by a config option. In this case the test
> should not return TCONF because a user could change the option and make
> themselves vulnerable.

I was looking for kernel config options, that could modify bpf behavior
and also unprivileged_bpf_disable (set to 0 for me), but nothing stood out.

> 
>> I guess what you wanted to check is what BPF_MAP_ARRAY_STX does. There
>> is one major difference between your implementation of the check and 
>> BPF_MAP_ARRAY_STX: BPF_MAP_ARRAY_STX uses an immediate value, to add
>> to r2 compared to a register in your check:
>>
>>  * r2 = fp
>>  * r2 += r2 - 4
>>
>> vs
>>
>>  * r2 = r10
>>  * r3 = -1
>>  * r2 += r3
>>
>> That is actually what makes the difference here...
> 
> No, BPF_MAP_ARRAY_STX is just used to output the result of div/mod by
> zero and I don't think it needs testing.
> 
>>
>> If I modify the check like this it works:
>>
>> --- a/testcases/kernel/syscalls/bpf/bpf_prog05.c
>> +++ b/testcases/kernel/syscalls/bpf/bpf_prog05.c
>> @@ -67,13 +67,11 @@ static void ensure_ptr_arithmetic(void)
>>  {
>>         const struct bpf_insn prog_insn[] = {
>>                 /* r2 = r10
>> -                * r3 = -1
>> -                * r2 += r3
>> +                * r2 += -1
>>                  * *(char *)r2 = 0
>>                  */
>>                 BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> -               BPF_MOV64_IMM(BPF_REG_3, -1),
>> -               BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
>> +               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -1),
>>                 BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0),
>>
>>                 /* exit(0) */
>>
>>
>> Is this still checking what it is supposed to? Then I would send it
>> post this as a patch
>>
>> Joerg
> 
> This will reintroduce the false positive.

Ok, I get that.


I did some more digging and bisecting. First I bisected the mainline
kernel and found, that the commit 2c78ee898 ("bpf: Implement CAP_BPF")
makes the check run successful. This is only in linux >= 5.8.
But my 5.4 ubuntu kernel also successfully ran the check, so I also
bisected ubuntu sources [1] and found this commit to be the fix here:
2fa9ab45c ("bpf: No need to simulate speculative domain for immediates")
This commit is also in the mainline kernel, but only in >= 5.13.

I guess the check you implemented now disables the test for a lot of
kernels, that do not have a patch like this... I will stop here and just
accept, that the test is not running successfully in my case. But still
wanted to share this information.

Joerg


[1]
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/commit/?id=2fa9ab45c53e8b104ba8f7d3a953131cc818fcc0



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 4/4] bpf_prog05: Drop CAP_BPF and check if ptr arithmetic is allowed

[Richard Palethorpe]

Hello Joerg,

> I did some more digging and bisecting. First I bisected the mainline
> kernel and found, that the commit 2c78ee898 ("bpf: Implement CAP_BPF")
> makes the check run successful. This is only in linux >= 5.8.
> But my 5.4 ubuntu kernel also successfully ran the check, so I also
> bisected ubuntu sources [1] and found this commit to be the fix here:
> 2fa9ab45c ("bpf: No need to simulate speculative domain for immediates")
> This commit is also in the mainline kernel, but only in >= 5.13.

Uffff, interesting, some of the things mentioned in this commit seem
familiar. I did start working on other BPF reproducers, but gave up for
the time being.

>
> I guess the check you implemented now disables the test for a lot of
> kernels, that do not have a patch like this... I will stop here and just
> accept, that the test is not running successfully in my case. But still
> wanted to share this information.

Thanks, yes this could be useful.

>
> Joerg
>
>
> [1]
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/commit/?id=2fa9ab45c53e8b104ba8f7d3a953131cc818fcc0


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp