Skip to contentSkip to conversation

bpf: fix misaligned access for BPF_PROG_TYPE_PERF_EVENT program type on x86_32 platform

PATCH In review Wang YanQing 3 participants 6 messages Apr 26, 2018 – May 7, 2018

Subsystems (6)
Networking Network drivers BPF Tracing +2 more subsystems
lore
Share & tools
lore

Wang YanQing

author
#0 lore raw

PATCH 1/1 bpf: fix misaligned access for BPF_PROG_TYPE_PERF_EVENT program type on x86_32 platform

All the testcases for BPF_PROG_TYPE_PERF_EVENT program type in test_verifier(kselftest) report below errors on x86_32:
"
172/p unpriv: spill/fill of different pointers ldx FAIL
Unexpected error message!
0: (bf) r6 = r10
1: (07) r6 += -8
2: (15) if r1 == 0x0 goto pc+3
R1=ctx(id=0,off=0,imm=0) R6=fp-8,call_-1 R10=fp0,call_-1
3: (bf) r2 = r10
4: (07) r2 += -76
5: (7b) *(u64 *)(r6 +0) = r2
6: (55) if r1 != 0x0 goto pc+1
R1=ctx(id=0,off=0,imm=0) R2=fp-76,call_-1 R6=fp-8,call_-1 R10=fp0,call_-1 fp-8=fp 7: (7b) *(u64 *)(r6 +0) = r1
8: (79) r1 = *(u64 *)(r6 +0)
9: (79) r1 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8

378/p check bpf_perf_event_data->sample_period byte load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (71) r0 = *(u8 *)(r1 +68)
invalid bpf_context access off=68 size=1

379/p check bpf_perf_event_data->sample_period half load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (69) r0 = *(u16 *)(r1 +68)
invalid bpf_context access off=68 size=2

380/p check bpf_perf_event_data->sample_period word load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (61) r0 = *(u32 *)(r1 +68)
invalid bpf_context access off=68 size=4

381/p check bpf_perf_event_data->sample_period dword load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (79) r0 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8
"

This patch fix it, the fix isn't only necessary for x86_32, it will fix the same problem for other platforms too, if their size of bpf_user_pt_regs_t can't divide exactly into 8.

Signed-off-by Wang YanQing <[email protected]>

---
Hi all!
After mainline accept this patch, then we need to submit a sync patch to update the tools/include/uapi/linux/bpf_perf_event.h.

Thanks.

 include/uapi/linux/bpf_perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/bpf_perf_event.h +1 -1
index eb1b9d2..ff4c092 100644
@@ -12,7 +12,7 @@
 
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
-	__u64 sample_period;
+	__u64 sample_period __attribute__((aligned(8)));
 	__u64 addr;
 };
Signature · 1 line
1.8.5.6.2.g3d8a54e.dirty
#1 lore raw
Quoted 64 lines · Wang YanQing

All the testcases for BPF_PROG_TYPE_PERF_EVENT program type in test_verifier(kselftest) report below errors on x86_32:
"
172/p unpriv: spill/fill of different pointers ldx FAIL
Unexpected error message!
0: (bf) r6 = r10
1: (07) r6 += -8
2: (15) if r1 == 0x0 goto pc+3
R1=ctx(id=0,off=0,imm=0) R6=fp-8,call_-1 R10=fp0,call_-1
3: (bf) r2 = r10
4: (07) r2 += -76
5: (7b) *(u64 *)(r6 +0) = r2
6: (55) if r1 != 0x0 goto pc+1
R1=ctx(id=0,off=0,imm=0) R2=fp-76,call_-1 R6=fp-8,call_-1 R10=fp0,call_-1 fp-8=fp 7: (7b) *(u64 *)(r6 +0) = r1
8: (79) r1 = *(u64 *)(r6 +0)
9: (79) r1 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8

378/p check bpf_perf_event_data->sample_period byte load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (71) r0 = *(u8 *)(r1 +68)
invalid bpf_context access off=68 size=1

379/p check bpf_perf_event_data->sample_period half load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (69) r0 = *(u16 *)(r1 +68)
invalid bpf_context access off=68 size=2

380/p check bpf_perf_event_data->sample_period word load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (61) r0 = *(u32 *)(r1 +68)
invalid bpf_context access off=68 size=4

381/p check bpf_perf_event_data->sample_period dword load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (79) r0 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8
"

This patch fix it, the fix isn't only necessary for x86_32, it will fix the same problem for other platforms too, if their size of bpf_user_pt_regs_t can't divide exactly into 8.

Signed-off-by Wang YanQing <[email protected]>

---
Hi all!
After mainline accept this patch, then we need to submit a sync patch to update the tools/include/uapi/linux/bpf_perf_event.h.

Thanks.

 include/uapi/linux/bpf_perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/bpf_perf_event.h +0 -0
index eb1b9d2..ff4c092 100644

include/uapi/linux/bpf_perf_event.h line 15

Quoted 5 lines
include/uapi/linux/bpf_perf_event.h +1 -1
@@ -12,7 +12,7 @@
 
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
-	__u64 sample_period;
+	__u64 sample_period __attribute__((aligned(8)));

I don't think this necessary.
imo it's a bug in pe_prog_is_valid_access
that should have allowed 8-byte access to 4-byte aligned sample_period. The access rewritten by pe_prog_convert_ctx_access anyway, no alignment issues as far as I can see.

maintainer
#2 lore raw
Quoted 66 lines · Alexei Starovoitov
Quoted 65 lines · Wang YanQing

All the testcases for BPF_PROG_TYPE_PERF_EVENT program type in test_verifier(kselftest) report below errors on x86_32:
"
172/p unpriv: spill/fill of different pointers ldx FAIL
Unexpected error message!
0: (bf) r6 = r10
1: (07) r6 += -8
2: (15) if r1 == 0x0 goto pc+3
R1=ctx(id=0,off=0,imm=0) R6=fp-8,call_-1 R10=fp0,call_-1
3: (bf) r2 = r10
4: (07) r2 += -76
5: (7b) *(u64 *)(r6 +0) = r2
6: (55) if r1 != 0x0 goto pc+1
R1=ctx(id=0,off=0,imm=0) R2=fp-76,call_-1 R6=fp-8,call_-1 R10=fp0,call_-1 fp-8=fp 7: (7b) *(u64 *)(r6 +0) = r1
8: (79) r1 = *(u64 *)(r6 +0)
9: (79) r1 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8

378/p check bpf_perf_event_data->sample_period byte load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (71) r0 = *(u8 *)(r1 +68)
invalid bpf_context access off=68 size=1

379/p check bpf_perf_event_data->sample_period half load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (69) r0 = *(u16 *)(r1 +68)
invalid bpf_context access off=68 size=2

380/p check bpf_perf_event_data->sample_period word load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (61) r0 = *(u32 *)(r1 +68)
invalid bpf_context access off=68 size=4

381/p check bpf_perf_event_data->sample_period dword load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (79) r0 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8
"

This patch fix it, the fix isn't only necessary for x86_32, it will fix the same problem for other platforms too, if their size of bpf_user_pt_regs_t can't divide exactly into 8.

Signed-off-by Wang YanQing <[email protected]>

---
Hi all!
After mainline accept this patch, then we need to submit a sync patch to update the tools/include/uapi/linux/bpf_perf_event.h.

Thanks.

 include/uapi/linux/bpf_perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/bpf_perf_event.h +0 -0
index eb1b9d2..ff4c092 100644
@@ -12,7 +12,7 @@
Quoted 10 lines

include/uapi/linux/bpf_perf_event.h line 15

Quoted 4 lines
include/uapi/linux/bpf_perf_event.h +1 -1
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
-	__u64 sample_period;
+	__u64 sample_period __attribute__((aligned(8)));

I don't think this necessary.
imo it's a bug in pe_prog_is_valid_access
that should have allowed 8-byte access to 4-byte aligned sample_period. The access rewritten by pe_prog_convert_ctx_access anyway, no alignment issues as far as I can see.

Right, good point. Wang, could you give the below a test run:

kernel/trace/bpf_trace.c +8 -2
index 56ba0f2..95b9142 100644
@@ -833,8 +833,14 @@ static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type
 		return false;
 	if (type != BPF_READ)
 		return false;
-	if (off % size != 0)
-		return false;
+	if (off % size != 0) {
+		if (sizeof(long) != 4)
+			return false;
+		if (size != 8)
+			return false;
+		if (off % size != 4)
+			return false;
+	}
 
 	switch (off) {
 	case bpf_ctx_range(struct bpf_perf_event_data, sample_period):
author
#3 lore raw
Quoted 91 lines · Daniel Borkmann
Quoted 66 lines · Alexei Starovoitov
Quoted 65 lines · Wang YanQing

All the testcases for BPF_PROG_TYPE_PERF_EVENT program type in test_verifier(kselftest) report below errors on x86_32:
"
172/p unpriv: spill/fill of different pointers ldx FAIL
Unexpected error message!
0: (bf) r6 = r10
1: (07) r6 += -8
2: (15) if r1 == 0x0 goto pc+3
R1=ctx(id=0,off=0,imm=0) R6=fp-8,call_-1 R10=fp0,call_-1
3: (bf) r2 = r10
4: (07) r2 += -76
5: (7b) *(u64 *)(r6 +0) = r2
6: (55) if r1 != 0x0 goto pc+1
R1=ctx(id=0,off=0,imm=0) R2=fp-76,call_-1 R6=fp-8,call_-1 R10=fp0,call_-1 fp-8=fp 7: (7b) *(u64 *)(r6 +0) = r1
8: (79) r1 = *(u64 *)(r6 +0)
9: (79) r1 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8

378/p check bpf_perf_event_data->sample_period byte load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (71) r0 = *(u8 *)(r1 +68)
invalid bpf_context access off=68 size=1

379/p check bpf_perf_event_data->sample_period half load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (69) r0 = *(u16 *)(r1 +68)
invalid bpf_context access off=68 size=2

380/p check bpf_perf_event_data->sample_period word load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (61) r0 = *(u32 *)(r1 +68)
invalid bpf_context access off=68 size=4

381/p check bpf_perf_event_data->sample_period dword load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (79) r0 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8
"

This patch fix it, the fix isn't only necessary for x86_32, it will fix the same problem for other platforms too, if their size of bpf_user_pt_regs_t can't divide exactly into 8.

Signed-off-by Wang YanQing <[email protected]>

---
Hi all!
After mainline accept this patch, then we need to submit a sync patch to update the tools/include/uapi/linux/bpf_perf_event.h.

Thanks.

 include/uapi/linux/bpf_perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/bpf_perf_event.h +0 -0
index eb1b9d2..ff4c092 100644
@@ -12,7 +12,7 @@
Quoted 10 lines

include/uapi/linux/bpf_perf_event.h line 15

Quoted 4 lines
include/uapi/linux/bpf_perf_event.h +1 -1
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
-	__u64 sample_period;
+	__u64 sample_period __attribute__((aligned(8)));

I don't think this necessary.
imo it's a bug in pe_prog_is_valid_access
that should have allowed 8-byte access to 4-byte aligned sample_period. The access rewritten by pe_prog_convert_ctx_access anyway, no alignment issues as far as I can see.

Right, good point. Wang, could you give the below a test run:

kernel/trace/bpf_trace.c +1 -2
index 56ba0f2..95b9142 100644
@@ -833,8 +833,14 @@ static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type
 		return false;
 	if (type != BPF_READ)
 		return false;
-	if (off % size != 0)
-		return false;
+	if (off % size != 0) {

kernel/trace/bpf_trace.c static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type · line 846

Quoted 10 lines
kernel/trace/bpf_trace.c +7 -0
+		if (sizeof(long) != 4)
+			return false;
+		if (size != 8)
+			return false;
+		if (off % size != 4)
+			return false;
+	}
 
 	switch (off) {
 	case bpf_ctx_range(struct bpf_perf_event_data, sample_period):

Hi all!

I have tested this patch, but test_verifier reports the same errors for the five testcases.

The reason is they all failed to pass the test of bpf_ctx_narrow_access_ok.

Thanks.

author
#4 lore raw
Quoted 104 lines · Wang YanQing
Quoted 91 lines · Daniel Borkmann
Quoted 66 lines · Alexei Starovoitov
Quoted 65 lines · Wang YanQing

All the testcases for BPF_PROG_TYPE_PERF_EVENT program type in test_verifier(kselftest) report below errors on x86_32:
"
172/p unpriv: spill/fill of different pointers ldx FAIL
Unexpected error message!
0: (bf) r6 = r10
1: (07) r6 += -8
2: (15) if r1 == 0x0 goto pc+3
R1=ctx(id=0,off=0,imm=0) R6=fp-8,call_-1 R10=fp0,call_-1
3: (bf) r2 = r10
4: (07) r2 += -76
5: (7b) *(u64 *)(r6 +0) = r2
6: (55) if r1 != 0x0 goto pc+1
R1=ctx(id=0,off=0,imm=0) R2=fp-76,call_-1 R6=fp-8,call_-1 R10=fp0,call_-1 fp-8=fp 7: (7b) *(u64 *)(r6 +0) = r1
8: (79) r1 = *(u64 *)(r6 +0)
9: (79) r1 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8

378/p check bpf_perf_event_data->sample_period byte load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (71) r0 = *(u8 *)(r1 +68)
invalid bpf_context access off=68 size=1

379/p check bpf_perf_event_data->sample_period half load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (69) r0 = *(u16 *)(r1 +68)
invalid bpf_context access off=68 size=2

380/p check bpf_perf_event_data->sample_period word load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (61) r0 = *(u32 *)(r1 +68)
invalid bpf_context access off=68 size=4

381/p check bpf_perf_event_data->sample_period dword load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (79) r0 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8
"

This patch fix it, the fix isn't only necessary for x86_32, it will fix the same problem for other platforms too, if their size of bpf_user_pt_regs_t can't divide exactly into 8.

Signed-off-by Wang YanQing <[email protected]>

---
Hi all!
After mainline accept this patch, then we need to submit a sync patch to update the tools/include/uapi/linux/bpf_perf_event.h.

Thanks.

 include/uapi/linux/bpf_perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/bpf_perf_event.h +0 -0
index eb1b9d2..ff4c092 100644
@@ -12,7 +12,7 @@
Quoted 10 lines

include/uapi/linux/bpf_perf_event.h line 15

Quoted 4 lines
include/uapi/linux/bpf_perf_event.h +1 -1
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
-	__u64 sample_period;
+	__u64 sample_period __attribute__((aligned(8)));

I don't think this necessary.
imo it's a bug in pe_prog_is_valid_access
that should have allowed 8-byte access to 4-byte aligned sample_period. The access rewritten by pe_prog_convert_ctx_access anyway, no alignment issues as far as I can see.

Right, good point. Wang, could you give the below a test run:

kernel/trace/bpf_trace.c +1 -2
index 56ba0f2..95b9142 100644
@@ -833,8 +833,14 @@ static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type
 		return false;
 	if (type != BPF_READ)
 		return false;
-	if (off % size != 0)
-		return false;
+	if (off % size != 0) {

kernel/trace/bpf_trace.c static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type · line 846

Quoted 10 lines
kernel/trace/bpf_trace.c +7 -0
+		if (sizeof(long) != 4)
+			return false;
+		if (size != 8)
+			return false;
+		if (off % size != 4)
+			return false;
+	}
 
 	switch (off) {
 	case bpf_ctx_range(struct bpf_perf_event_data, sample_period):

Hi all!

Quoted 6 lines

I have tested this patch, but test_verifier reports the same errors for the five testcases.

The reason is they all failed to pass the test of bpf_ctx_narrow_access_ok.

Thanks.

Hi! Daniel Borkmann.

Do you have any plan to fix bpf_ctx_narrow_access_ok for these problems?

Thanks.

maintainer
#5 lore raw
Quoted 108 lines · Wang YanQing
Quoted 107 lines · Wang YanQing
Quoted 91 lines · Daniel Borkmann
Quoted 66 lines · Alexei Starovoitov
Quoted 65 lines · Wang YanQing

All the testcases for BPF_PROG_TYPE_PERF_EVENT program type in test_verifier(kselftest) report below errors on x86_32:
"
172/p unpriv: spill/fill of different pointers ldx FAIL
Unexpected error message!
0: (bf) r6 = r10
1: (07) r6 += -8
2: (15) if r1 == 0x0 goto pc+3
R1=ctx(id=0,off=0,imm=0) R6=fp-8,call_-1 R10=fp0,call_-1
3: (bf) r2 = r10
4: (07) r2 += -76
5: (7b) *(u64 *)(r6 +0) = r2
6: (55) if r1 != 0x0 goto pc+1
R1=ctx(id=0,off=0,imm=0) R2=fp-76,call_-1 R6=fp-8,call_-1 R10=fp0,call_-1 fp-8=fp 7: (7b) *(u64 *)(r6 +0) = r1
8: (79) r1 = *(u64 *)(r6 +0)
9: (79) r1 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8

378/p check bpf_perf_event_data->sample_period byte load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (71) r0 = *(u8 *)(r1 +68)
invalid bpf_context access off=68 size=1

379/p check bpf_perf_event_data->sample_period half load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (69) r0 = *(u16 *)(r1 +68)
invalid bpf_context access off=68 size=2

380/p check bpf_perf_event_data->sample_period word load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (61) r0 = *(u32 *)(r1 +68)
invalid bpf_context access off=68 size=4

381/p check bpf_perf_event_data->sample_period dword load permitted FAIL Failed to load prog 'Permission denied'!
0: (b7) r0 = 0
1: (79) r0 = *(u64 *)(r1 +68)
invalid bpf_context access off=68 size=8
"

This patch fix it, the fix isn't only necessary for x86_32, it will fix the same problem for other platforms too, if their size of bpf_user_pt_regs_t can't divide exactly into 8.

Signed-off-by Wang YanQing <[email protected]>

---
Hi all!
After mainline accept this patch, then we need to submit a sync patch to update the tools/include/uapi/linux/bpf_perf_event.h.

Thanks.

 include/uapi/linux/bpf_perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/bpf_perf_event.h +0 -0
index eb1b9d2..ff4c092 100644
@@ -12,7 +12,7 @@
Quoted 10 lines

include/uapi/linux/bpf_perf_event.h line 15

Quoted 4 lines
include/uapi/linux/bpf_perf_event.h +1 -1
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
-	__u64 sample_period;
+	__u64 sample_period __attribute__((aligned(8)));

I don't think this necessary.
imo it's a bug in pe_prog_is_valid_access
that should have allowed 8-byte access to 4-byte aligned sample_period. The access rewritten by pe_prog_convert_ctx_access anyway, no alignment issues as far as I can see.

Right, good point. Wang, could you give the below a test run:

kernel/trace/bpf_trace.c +1 -2
index 56ba0f2..95b9142 100644
@@ -833,8 +833,14 @@ static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type
 		return false;
 	if (type != BPF_READ)
 		return false;
-	if (off % size != 0)
-		return false;
+	if (off % size != 0) {

kernel/trace/bpf_trace.c static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type · line 846

Quoted 10 lines
kernel/trace/bpf_trace.c +7 -0
+		if (sizeof(long) != 4)
+			return false;
+		if (size != 8)
+			return false;
+		if (off % size != 4)
+			return false;
+	}
 
 	switch (off) {
 	case bpf_ctx_range(struct bpf_perf_event_data, sample_period):

Hi all!

I have tested this patch, but test_verifier reports the same errors for the five testcases.

Quoted 6 lines
Quoted 3 lines

The reason is they all failed to pass the test of bpf_ctx_narrow_access_ok.

Thanks.

Hi! Daniel Borkmann.

Do you have any plan to fix bpf_ctx_narrow_access_ok for these problems?

Yep, sorry for the delay, will get to it during this week.

Thanks,
Daniel