Re: [PATCH net-next] ebpf: Allow dereferences of PTR_TO_STACK registers
From: Alex Gartrell agartr...@fb.com Date: Thu, 23 Jul 2015 14:24:40 -0700 mov %rsp, %r1 ; r1 = rsp add $-8, %r1; r1 = rsp - 8 store_q $123, -8(%rsp) ; *(u64*)r1 = 123 - valid store_q $123, (%r1) ; *(u64*)r1 = 123 - previously invalid mov $0, %r0 exit; Always need to exit And we'd get the following error: 0: (bf) r1 = r10 1: (07) r1 += -8 2: (7a) *(u64 *)(r10 -8) = 999 3: (7a) *(u64 *)(r1 +0) = 999 R1 invalid mem access 'fp' Unable to load program We already know that a register is a stack address and the appropriate offset, so we should be able to validate those references as well. Signed-off-by: Alex Gartrell agartr...@fb.com Applied, thanks. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] ebpf: Allow dereferences of PTR_TO_STACK registers
mov %rsp, %r1 ; r1 = rsp
add $-8, %r1; r1 = rsp - 8
store_q $123, -8(%rsp) ; *(u64*)r1 = 123 - valid
store_q $123, (%r1) ; *(u64*)r1 = 123 - previously invalid
mov $0, %r0
exit; Always need to exit
And we'd get the following error:
0: (bf) r1 = r10
1: (07) r1 += -8
2: (7a) *(u64 *)(r10 -8) = 999
3: (7a) *(u64 *)(r1 +0) = 999
R1 invalid mem access 'fp'
Unable to load program
We already know that a register is a stack address and the appropriate
offset, so we should be able to validate those references as well.
Signed-off-by: Alex Gartrell agartr...@fb.com
---
kernel/bpf/verifier.c | 6 -
samples/bpf/test_verifier.c | 59 +
2 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 039d866..cd307df 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -648,6 +648,9 @@ static int check_mem_access(struct verifier_env *env, u32
regno, int off,
struct verifier_state *state = env-cur_state;
int size, err = 0;
+ if (state-regs[regno].type == PTR_TO_STACK)
+ off += state-regs[regno].imm;
+
size = bpf_size_to_bytes(bpf_size);
if (size 0)
return size;
@@ -667,7 +670,8 @@ static int check_mem_access(struct verifier_env *env, u32
regno, int off,
if (!err t == BPF_READ value_regno = 0)
mark_reg_unknown_value(state-regs, value_regno);
- } else if (state-regs[regno].type == FRAME_PTR) {
+ } else if (state-regs[regno].type == FRAME_PTR ||
+ state-regs[regno].type == PTR_TO_STACK) {
if (off = 0 || off -MAX_BPF_STACK) {
verbose(invalid stack off=%d size=%d\n, off, size);
return -EACCES;
diff --git a/samples/bpf/test_verifier.c b/samples/bpf/test_verifier.c
index 6936059..ee0f110 100644
--- a/samples/bpf/test_verifier.c
+++ b/samples/bpf/test_verifier.c
@@ -822,6 +822,65 @@ static struct bpf_test tests[] = {
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
+ {
+ PTR_TO_STACK store/load,
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -10),
+ BPF_ST_MEM(BPF_DW, BPF_REG_1, 2, 0xfaceb00c),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 2),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ },
+ {
+ PTR_TO_STACK store/load - bad alignment on off,
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_1, 2, 0xfaceb00c),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 2),
+ BPF_EXIT_INSN(),
+ },
+ .result = REJECT,
+ .errstr = misaligned access off -6 size 8,
+ },
+ {
+ PTR_TO_STACK store/load - bad alignment on reg,
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -10),
+ BPF_ST_MEM(BPF_DW, BPF_REG_1, 8, 0xfaceb00c),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 8),
+ BPF_EXIT_INSN(),
+ },
+ .result = REJECT,
+ .errstr = misaligned access off -2 size 8,
+ },
+ {
+ PTR_TO_STACK store/load - out of bounds low,
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_1, 8, 0xfaceb00c),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 8),
+ BPF_EXIT_INSN(),
+ },
+ .result = REJECT,
+ .errstr = invalid stack off=-79992 size=8,
+ },
+ {
+ PTR_TO_STACK store/load - out of bounds high,
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_1, 8, 0xfaceb00c),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 8),
+ BPF_EXIT_INSN(),
+ },
+ .result = REJECT,
+ .errstr = invalid stack off=0 size=8,
+ },
};
static int probe_filter_length(struct bpf_insn *fp)
--
Alex Gartrell agartr...@fb.com
--
To unsubscribe from this list:
Re: [PATCH net-next] ebpf: Allow dereferences of PTR_TO_STACK registers
On Thu, Jul 23, 2015 at 02:24:40PM -0700, Alex Gartrell wrote: mov %rsp, %r1 ; r1 = rsp add $-8, %r1; r1 = rsp - 8 store_q $123, -8(%rsp) ; *(u64*)r1 = 123 - valid store_q $123, (%r1) ; *(u64*)r1 = 123 - previously invalid mov $0, %r0 exit; Always need to exit And we'd get the following error: 0: (bf) r1 = r10 1: (07) r1 += -8 2: (7a) *(u64 *)(r10 -8) = 999 3: (7a) *(u64 *)(r1 +0) = 999 R1 invalid mem access 'fp' Unable to load program We already know that a register is a stack address and the appropriate offset, so we should be able to validate those references as well. Signed-off-by: Alex Gartrell agartr...@fb.com --- kernel/bpf/verifier.c | 6 - samples/bpf/test_verifier.c | 59 + 2 files changed, 64 insertions(+), 1 deletion(-) Looks good. Acked-by: Alexei Starovoitov a...@plumgrid.com + BPF_ST_MEM(BPF_DW, BPF_REG_1, 2, 0xfaceb00c), nice constants :) -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH net-next] ebpf: Allow dereferences of PTR_TO_STACK registers
On Tue, Jul 21, 2015 at 8:00 PM, Alexei Starovoitov a...@kernel.org wrote: On Tue, Jul 21, 2015 at 07:00:40PM -0700, Alex Gartrell wrote: mov %rsp, %r1 ; r1 = rsp add $-8, %r1; r1 = rsp - 8 store_q $123, -8(%rsp) ; *(u64*)r1 = 123 - valid store_q $123, (%r1) ; *(u64*)r1 = 123 - previously invalid mov $0, %r0 exit; Always need to exit Is this your new eBPF assembler syntax? :) imo gnu style looks ugly... ;) If you think this is ugly, you'll love the instruction I added to be compatible with the map fd - immediate conversion hack :) It's great to see such in-depth understanding of verifier!! And we'd get the following error: 0: (bf) r1 = r10 1: (07) r1 += -8 2: (7a) *(u64 *)(r10 -8) = 999 3: (7a) *(u64 *)(r1 +0) = 999 R1 invalid mem access 'fp' Unable to load program We already know that a register is a stack address and the appropriate offset, so we should be able to validate those references as well. yes, we can teach verifier to do that. Though llvm doesn't generate such code. It's small enough change. I happened upon this as I was playing around with the bytecode in our 4.0 kernels. I believe that we can write general purpose utilities without needing to write C code for each use case that do things like filtering/counting packets or syscalls and outputting that data into maps at low cost, but I'm still just prototyping so I'm not ready to be an assertive jerk about it (yet) real_off is missing alignment and bounds checks. something like: if (state-regs[regno].type == PTR_TO_STACK) off += state-regs[regno].imm; if (off % size != 0) ... Yeah, I'm an idiot and assumed that a bounds check happened in the check_stack_read function. I'll find a way to do this without copy-pasta'ing but I'm going to stick to my moral high ground and not mutate a parameter (this lead to a bug in a job interview 6 years ago and I've never forgiven myself because the interviewer was an OpenBSD guy) else if (state-regs[regno].type == FRAME_PTR || == PTR_TO_STACK) .. as-is here ... would fix it. please add few accept and reject tests for this to test_verifier.c as well. psh, tests... I'll update this stuff and submit a patch. -- Alex Gartrell agartr...@fb.com -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH net-next] ebpf: Allow dereferences of PTR_TO_STACK registers
On Tue, Jul 21, 2015 at 07:00:40PM -0700, Alex Gartrell wrote:
mov %rsp, %r1 ; r1 = rsp
add $-8, %r1; r1 = rsp - 8
store_q $123, -8(%rsp) ; *(u64*)r1 = 123 - valid
store_q $123, (%r1) ; *(u64*)r1 = 123 - previously invalid
mov $0, %r0
exit; Always need to exit
Is this your new eBPF assembler syntax? :)
imo gnu style looks ugly... ;)
It's great to see such in-depth understanding of verifier!!
And we'd get the following error:
0: (bf) r1 = r10
1: (07) r1 += -8
2: (7a) *(u64 *)(r10 -8) = 999
3: (7a) *(u64 *)(r1 +0) = 999
R1 invalid mem access 'fp'
Unable to load program
We already know that a register is a stack address and the appropriate
offset, so we should be able to validate those references as well.
yes, we can teach verifier to do that.
Though llvm doesn't generate such code. It's small enough change.
Signed-off-by: Alex Gartrell agartr...@fb.com
---
kernel/bpf/verifier.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 039d866..5dfbece 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -676,6 +676,15 @@ static int check_mem_access(struct verifier_env *env,
u32 regno, int off,
err = check_stack_write(state, off, size, value_regno);
else
err = check_stack_read(state, off, size, value_regno);
+ } else if (state-regs[regno].type == PTR_TO_STACK) {
+ int real_off = state-regs[regno].imm + off;
real_off is missing alignment and bounds checks.
something like:
if (state-regs[regno].type == PTR_TO_STACK)
off += state-regs[regno].imm;
if (off % size != 0)
...
else if (state-regs[regno].type == FRAME_PTR || == PTR_TO_STACK)
.. as-is here ...
would fix it.
please add few accept and reject tests for this to test_verifier.c as well.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH net-next] ebpf: Allow dereferences of PTR_TO_STACK registers
mov %rsp, %r1 ; r1 = rsp
add $-8, %r1; r1 = rsp - 8
store_q $123, -8(%rsp) ; *(u64*)r1 = 123 - valid
store_q $123, (%r1) ; *(u64*)r1 = 123 - previously invalid
mov $0, %r0
exit; Always need to exit
And we'd get the following error:
0: (bf) r1 = r10
1: (07) r1 += -8
2: (7a) *(u64 *)(r10 -8) = 999
3: (7a) *(u64 *)(r1 +0) = 999
R1 invalid mem access 'fp'
Unable to load program
We already know that a register is a stack address and the appropriate
offset, so we should be able to validate those references as well.
Signed-off-by: Alex Gartrell agartr...@fb.com
---
kernel/bpf/verifier.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 039d866..5dfbece 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -676,6 +676,15 @@ static int check_mem_access(struct verifier_env *env, u32
regno, int off,
err = check_stack_write(state, off, size, value_regno);
else
err = check_stack_read(state, off, size, value_regno);
+ } else if (state-regs[regno].type == PTR_TO_STACK) {
+ int real_off = state-regs[regno].imm + off;
+
+ if (t == BPF_WRITE)
+ err = check_stack_write(
+ state, real_off, size, value_regno);
+ else
+ err = check_stack_read(
+ state, real_off, size, value_regno);
} else {
verbose(R%d invalid mem access '%s'\n,
regno, reg_type_str[state-regs[regno].type]);
--
Alex Gartrell agartr...@fb.com
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
