diff options
Diffstat (limited to 'common/recipes-kernel/linux/linux-yocto-4.9.21/0093-bpf-fix-mixed-signed-unsigned-derived-min-max-value-.patch')
| -rw-r--r-- | common/recipes-kernel/linux/linux-yocto-4.9.21/0093-bpf-fix-mixed-signed-unsigned-derived-min-max-value-.patch | 463 |
1 files changed, 463 insertions, 0 deletions
diff --git a/common/recipes-kernel/linux/linux-yocto-4.9.21/0093-bpf-fix-mixed-signed-unsigned-derived-min-max-value-.patch b/common/recipes-kernel/linux/linux-yocto-4.9.21/0093-bpf-fix-mixed-signed-unsigned-derived-min-max-value-.patch new file mode 100644 index 00000000..8dce43f7 --- /dev/null +++ b/common/recipes-kernel/linux/linux-yocto-4.9.21/0093-bpf-fix-mixed-signed-unsigned-derived-min-max-value-.patch @@ -0,0 +1,463 @@ +From ae18a063a2a05514cf0821c68eecf75831c6200f Mon Sep 17 00:00:00 2001 +From: Daniel Borkmann <daniel@iogearbox.net> +Date: Fri, 21 Jul 2017 00:00:21 +0200 +Subject: [PATCH 093/102] bpf: fix mixed signed/unsigned derived min/max value + bounds + +[ Upstream commit 4cabc5b186b5427b9ee5a7495172542af105f02b ] + +Edward reported that there's an issue in min/max value bounds +tracking when signed and unsigned compares both provide hints +on limits when having unknown variables. E.g. a program such +as the following should have been rejected: + + 0: (7a) *(u64 *)(r10 -8) = 0 + 1: (bf) r2 = r10 + 2: (07) r2 += -8 + 3: (18) r1 = 0xffff8a94cda93400 + 5: (85) call bpf_map_lookup_elem#1 + 6: (15) if r0 == 0x0 goto pc+7 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R10=fp + 7: (7a) *(u64 *)(r10 -16) = -8 + 8: (79) r1 = *(u64 *)(r10 -16) + 9: (b7) r2 = -1 + 10: (2d) if r1 > r2 goto pc+3 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R1=inv,min_value=0 + R2=imm-1,max_value=18446744073709551615,min_align=1 R10=fp + 11: (65) if r1 s> 0x1 goto pc+2 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R1=inv,min_value=0,max_value=1 + R2=imm-1,max_value=18446744073709551615,min_align=1 R10=fp + 12: (0f) r0 += r1 + 13: (72) *(u8 *)(r0 +0) = 0 + R0=map_value_adj(ks=8,vs=8,id=0),min_value=0,max_value=1 R1=inv,min_value=0,max_value=1 + R2=imm-1,max_value=18446744073709551615,min_align=1 R10=fp + 14: (b7) r0 = 0 + 15: (95) exit + +What happens is that in the first part ... + + 8: (79) r1 = *(u64 *)(r10 -16) + 9: (b7) r2 = -1 + 10: (2d) if r1 > r2 goto pc+3 + +... r1 carries an unsigned value, and is compared as unsigned +against a register carrying an immediate. Verifier deduces in +reg_set_min_max() that since the compare is unsigned and operation +is greater than (>), that in the fall-through/false case, r1's +minimum bound must be 0 and maximum bound must be r2. Latter is +larger than the bound and thus max value is reset back to being +'invalid' aka BPF_REGISTER_MAX_RANGE. Thus, r1 state is now +'R1=inv,min_value=0'. The subsequent test ... + + 11: (65) if r1 s> 0x1 goto pc+2 + +... is a signed compare of r1 with immediate value 1. Here, +verifier deduces in reg_set_min_max() that since the compare +is signed this time and operation is greater than (>), that +in the fall-through/false case, we can deduce that r1's maximum +bound must be 1, meaning with prior test, we result in r1 having +the following state: R1=inv,min_value=0,max_value=1. Given that +the actual value this holds is -8, the bounds are wrongly deduced. +When this is being added to r0 which holds the map_value(_adj) +type, then subsequent store access in above case will go through +check_mem_access() which invokes check_map_access_adj(), that +will then probe whether the map memory is in bounds based +on the min_value and max_value as well as access size since +the actual unknown value is min_value <= x <= max_value; commit +fce366a9dd0d ("bpf, verifier: fix alu ops against map_value{, +_adj} register types") provides some more explanation on the +semantics. + +It's worth to note in this context that in the current code, +min_value and max_value tracking are used for two things, i) +dynamic map value access via check_map_access_adj() and since +commit 06c1c049721a ("bpf: allow helpers access to variable memory") +ii) also enforced at check_helper_mem_access() when passing a +memory address (pointer to packet, map value, stack) and length +pair to a helper and the length in this case is an unknown value +defining an access range through min_value/max_value in that +case. The min_value/max_value tracking is /not/ used in the +direct packet access case to track ranges. However, the issue +also affects case ii), for example, the following crafted program +based on the same principle must be rejected as well: + + 0: (b7) r2 = 0 + 1: (bf) r3 = r10 + 2: (07) r3 += -512 + 3: (7a) *(u64 *)(r10 -16) = -8 + 4: (79) r4 = *(u64 *)(r10 -16) + 5: (b7) r6 = -1 + 6: (2d) if r4 > r6 goto pc+5 + R1=ctx R2=imm0,min_value=0,max_value=0,min_align=2147483648 R3=fp-512 + R4=inv,min_value=0 R6=imm-1,max_value=18446744073709551615,min_align=1 R10=fp + 7: (65) if r4 s> 0x1 goto pc+4 + R1=ctx R2=imm0,min_value=0,max_value=0,min_align=2147483648 R3=fp-512 + R4=inv,min_value=0,max_value=1 R6=imm-1,max_value=18446744073709551615,min_align=1 + R10=fp + 8: (07) r4 += 1 + 9: (b7) r5 = 0 + 10: (6a) *(u16 *)(r10 -512) = 0 + 11: (85) call bpf_skb_load_bytes#26 + 12: (b7) r0 = 0 + 13: (95) exit + +Meaning, while we initialize the max_value stack slot that the +verifier thinks we access in the [1,2] range, in reality we +pass -7 as length which is interpreted as u32 in the helper. +Thus, this issue is relevant also for the case of helper ranges. +Resetting both bounds in check_reg_overflow() in case only one +of them exceeds limits is also not enough as similar test can be +created that uses values which are within range, thus also here +learned min value in r1 is incorrect when mixed with later signed +test to create a range: + + 0: (7a) *(u64 *)(r10 -8) = 0 + 1: (bf) r2 = r10 + 2: (07) r2 += -8 + 3: (18) r1 = 0xffff880ad081fa00 + 5: (85) call bpf_map_lookup_elem#1 + 6: (15) if r0 == 0x0 goto pc+7 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R10=fp + 7: (7a) *(u64 *)(r10 -16) = -8 + 8: (79) r1 = *(u64 *)(r10 -16) + 9: (b7) r2 = 2 + 10: (3d) if r2 >= r1 goto pc+3 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R1=inv,min_value=3 + R2=imm2,min_value=2,max_value=2,min_align=2 R10=fp + 11: (65) if r1 s> 0x4 goto pc+2 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 + R1=inv,min_value=3,max_value=4 R2=imm2,min_value=2,max_value=2,min_align=2 R10=fp + 12: (0f) r0 += r1 + 13: (72) *(u8 *)(r0 +0) = 0 + R0=map_value_adj(ks=8,vs=8,id=0),min_value=3,max_value=4 + R1=inv,min_value=3,max_value=4 R2=imm2,min_value=2,max_value=2,min_align=2 R10=fp + 14: (b7) r0 = 0 + 15: (95) exit + +This leaves us with two options for fixing this: i) to invalidate +all prior learned information once we switch signed context, ii) +to track min/max signed and unsigned boundaries separately as +done in [0]. (Given latter introduces major changes throughout +the whole verifier, it's rather net-next material, thus this +patch follows option i), meaning we can derive bounds either +from only signed tests or only unsigned tests.) There is still the +case of adjust_reg_min_max_vals(), where we adjust bounds on ALU +operations, meaning programs like the following where boundaries +on the reg get mixed in context later on when bounds are merged +on the dst reg must get rejected, too: + + 0: (7a) *(u64 *)(r10 -8) = 0 + 1: (bf) r2 = r10 + 2: (07) r2 += -8 + 3: (18) r1 = 0xffff89b2bf87ce00 + 5: (85) call bpf_map_lookup_elem#1 + 6: (15) if r0 == 0x0 goto pc+6 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R10=fp + 7: (7a) *(u64 *)(r10 -16) = -8 + 8: (79) r1 = *(u64 *)(r10 -16) + 9: (b7) r2 = 2 + 10: (3d) if r2 >= r1 goto pc+2 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R1=inv,min_value=3 + R2=imm2,min_value=2,max_value=2,min_align=2 R10=fp + 11: (b7) r7 = 1 + 12: (65) if r7 s> 0x0 goto pc+2 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R1=inv,min_value=3 + R2=imm2,min_value=2,max_value=2,min_align=2 R7=imm1,max_value=0 R10=fp + 13: (b7) r0 = 0 + 14: (95) exit + + from 12 to 15: R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 + R1=inv,min_value=3 R2=imm2,min_value=2,max_value=2,min_align=2 R7=imm1,min_value=1 R10=fp + 15: (0f) r7 += r1 + 16: (65) if r7 s> 0x4 goto pc+2 + R0=map_value(ks=8,vs=8,id=0),min_value=0,max_value=0 R1=inv,min_value=3 + R2=imm2,min_value=2,max_value=2,min_align=2 R7=inv,min_value=4,max_value=4 R10=fp + 17: (0f) r0 += r7 + 18: (72) *(u8 *)(r0 +0) = 0 + R0=map_value_adj(ks=8,vs=8,id=0),min_value=4,max_value=4 R1=inv,min_value=3 + R2=imm2,min_value=2,max_value=2,min_align=2 R7=inv,min_value=4,max_value=4 R10=fp + 19: (b7) r0 = 0 + 20: (95) exit + +Meaning, in adjust_reg_min_max_vals() we must also reset range +values on the dst when src/dst registers have mixed signed/ +unsigned derived min/max value bounds with one unbounded value +as otherwise they can be added together deducing false boundaries. +Once both boundaries are established from either ALU ops or +compare operations w/o mixing signed/unsigned insns, then they +can safely be added to other regs also having both boundaries +established. Adding regs with one unbounded side to a map value +where the bounded side has been learned w/o mixing ops is +possible, but the resulting map value won't recover from that, +meaning such op is considered invalid on the time of actual +access. Invalid bounds are set on the dst reg in case i) src reg, +or ii) in case dst reg already had them. The only way to recover +would be to perform i) ALU ops but only 'add' is allowed on map +value types or ii) comparisons, but these are disallowed on +pointers in case they span a range. This is fine as only BPF_JEQ +and BPF_JNE may be performed on PTR_TO_MAP_VALUE_OR_NULL registers +which potentially turn them into PTR_TO_MAP_VALUE type depending +on the branch, so only here min/max value cannot be invalidated +for them. + +In terms of state pruning, value_from_signed is considered +as well in states_equal() when dealing with adjusted map values. +With regards to breaking existing programs, there is a small +risk, but use-cases are rather quite narrow where this could +occur and mixing compares probably unlikely. + +Joint work with Josef and Edward. + + [0] https://lists.iovisor.org/pipermail/iovisor-dev/2017-June/000822.html + +Fixes: 484611357c19 ("bpf: allow access into map value arrays") +Reported-by: Edward Cree <ecree@solarflare.com> +Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> +Signed-off-by: Edward Cree <ecree@solarflare.com> +Signed-off-by: Josef Bacik <jbacik@fb.com> +Signed-off-by: David S. Miller <davem@davemloft.net> +Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> +--- + include/linux/bpf_verifier.h | 1 + + kernel/bpf/verifier.c | 110 +++++++++++++++++++++++++++++++++++++------ + 2 files changed, 97 insertions(+), 14 deletions(-) + +diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h +index 2edf8de..070fc49 100644 +--- a/include/linux/bpf_verifier.h ++++ b/include/linux/bpf_verifier.h +@@ -40,6 +40,7 @@ struct bpf_reg_state { + */ + s64 min_value; + u64 max_value; ++ bool value_from_signed; + }; + + enum bpf_stack_slot_type { +diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c +index 56a867f..5f274c6 100644 +--- a/kernel/bpf/verifier.c ++++ b/kernel/bpf/verifier.c +@@ -671,12 +671,13 @@ static int check_ctx_access(struct bpf_verifier_env *env, int off, int size, + return -EACCES; + } + +-static bool is_pointer_value(struct bpf_verifier_env *env, int regno) ++static bool __is_pointer_value(bool allow_ptr_leaks, ++ const struct bpf_reg_state *reg) + { +- if (env->allow_ptr_leaks) ++ if (allow_ptr_leaks) + return false; + +- switch (env->cur_state.regs[regno].type) { ++ switch (reg->type) { + case UNKNOWN_VALUE: + case CONST_IMM: + return false; +@@ -685,6 +686,11 @@ static bool is_pointer_value(struct bpf_verifier_env *env, int regno) + } + } + ++static bool is_pointer_value(struct bpf_verifier_env *env, int regno) ++{ ++ return __is_pointer_value(env->allow_ptr_leaks, &env->cur_state.regs[regno]); ++} ++ + static int check_ptr_alignment(struct bpf_verifier_env *env, + struct bpf_reg_state *reg, int off, int size) + { +@@ -1521,10 +1527,24 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env, + } + + /* We don't know anything about what was done to this register, mark it +- * as unknown. ++ * as unknown. Also, if both derived bounds came from signed/unsigned ++ * mixed compares and one side is unbounded, we cannot really do anything ++ * with them as boundaries cannot be trusted. Thus, arithmetic of two ++ * regs of such kind will get invalidated bounds on the dst side. + */ +- if (min_val == BPF_REGISTER_MIN_RANGE && +- max_val == BPF_REGISTER_MAX_RANGE) { ++ if ((min_val == BPF_REGISTER_MIN_RANGE && ++ max_val == BPF_REGISTER_MAX_RANGE) || ++ (BPF_SRC(insn->code) == BPF_X && ++ ((min_val != BPF_REGISTER_MIN_RANGE && ++ max_val == BPF_REGISTER_MAX_RANGE) || ++ (min_val == BPF_REGISTER_MIN_RANGE && ++ max_val != BPF_REGISTER_MAX_RANGE) || ++ (dst_reg->min_value != BPF_REGISTER_MIN_RANGE && ++ dst_reg->max_value == BPF_REGISTER_MAX_RANGE) || ++ (dst_reg->min_value == BPF_REGISTER_MIN_RANGE && ++ dst_reg->max_value != BPF_REGISTER_MAX_RANGE)) && ++ regs[insn->dst_reg].value_from_signed != ++ regs[insn->src_reg].value_from_signed)) { + reset_reg_range_values(regs, insn->dst_reg); + return; + } +@@ -1855,38 +1875,63 @@ static void reg_set_min_max(struct bpf_reg_state *true_reg, + struct bpf_reg_state *false_reg, u64 val, + u8 opcode) + { ++ bool value_from_signed = true; ++ bool is_range = true; ++ + switch (opcode) { + case BPF_JEQ: + /* If this is false then we know nothing Jon Snow, but if it is + * true then we know for sure. + */ + true_reg->max_value = true_reg->min_value = val; ++ is_range = false; + break; + case BPF_JNE: + /* If this is true we know nothing Jon Snow, but if it is false + * we know the value for sure; + */ + false_reg->max_value = false_reg->min_value = val; ++ is_range = false; + break; + case BPF_JGT: +- /* Unsigned comparison, the minimum value is 0. */ +- false_reg->min_value = 0; ++ value_from_signed = false; ++ /* fallthrough */ + case BPF_JSGT: ++ if (true_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(true_reg, 0); ++ if (false_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(false_reg, 0); ++ if (opcode == BPF_JGT) { ++ /* Unsigned comparison, the minimum value is 0. */ ++ false_reg->min_value = 0; ++ } + /* If this is false then we know the maximum val is val, + * otherwise we know the min val is val+1. + */ + false_reg->max_value = val; ++ false_reg->value_from_signed = value_from_signed; + true_reg->min_value = val + 1; ++ true_reg->value_from_signed = value_from_signed; + break; + case BPF_JGE: +- /* Unsigned comparison, the minimum value is 0. */ +- false_reg->min_value = 0; ++ value_from_signed = false; ++ /* fallthrough */ + case BPF_JSGE: ++ if (true_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(true_reg, 0); ++ if (false_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(false_reg, 0); ++ if (opcode == BPF_JGE) { ++ /* Unsigned comparison, the minimum value is 0. */ ++ false_reg->min_value = 0; ++ } + /* If this is false then we know the maximum value is val - 1, + * otherwise we know the mimimum value is val. + */ + false_reg->max_value = val - 1; ++ false_reg->value_from_signed = value_from_signed; + true_reg->min_value = val; ++ true_reg->value_from_signed = value_from_signed; + break; + default: + break; +@@ -1894,6 +1939,12 @@ static void reg_set_min_max(struct bpf_reg_state *true_reg, + + check_reg_overflow(false_reg); + check_reg_overflow(true_reg); ++ if (is_range) { ++ if (__is_pointer_value(false, false_reg)) ++ reset_reg_range_values(false_reg, 0); ++ if (__is_pointer_value(false, true_reg)) ++ reset_reg_range_values(true_reg, 0); ++ } + } + + /* Same as above, but for the case that dst_reg is a CONST_IMM reg and src_reg +@@ -1903,39 +1954,64 @@ static void reg_set_min_max_inv(struct bpf_reg_state *true_reg, + struct bpf_reg_state *false_reg, u64 val, + u8 opcode) + { ++ bool value_from_signed = true; ++ bool is_range = true; ++ + switch (opcode) { + case BPF_JEQ: + /* If this is false then we know nothing Jon Snow, but if it is + * true then we know for sure. + */ + true_reg->max_value = true_reg->min_value = val; ++ is_range = false; + break; + case BPF_JNE: + /* If this is true we know nothing Jon Snow, but if it is false + * we know the value for sure; + */ + false_reg->max_value = false_reg->min_value = val; ++ is_range = false; + break; + case BPF_JGT: +- /* Unsigned comparison, the minimum value is 0. */ +- true_reg->min_value = 0; ++ value_from_signed = false; ++ /* fallthrough */ + case BPF_JSGT: ++ if (true_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(true_reg, 0); ++ if (false_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(false_reg, 0); ++ if (opcode == BPF_JGT) { ++ /* Unsigned comparison, the minimum value is 0. */ ++ true_reg->min_value = 0; ++ } + /* + * If this is false, then the val is <= the register, if it is + * true the register <= to the val. + */ + false_reg->min_value = val; ++ false_reg->value_from_signed = value_from_signed; + true_reg->max_value = val - 1; ++ true_reg->value_from_signed = value_from_signed; + break; + case BPF_JGE: +- /* Unsigned comparison, the minimum value is 0. */ +- true_reg->min_value = 0; ++ value_from_signed = false; ++ /* fallthrough */ + case BPF_JSGE: ++ if (true_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(true_reg, 0); ++ if (false_reg->value_from_signed != value_from_signed) ++ reset_reg_range_values(false_reg, 0); ++ if (opcode == BPF_JGE) { ++ /* Unsigned comparison, the minimum value is 0. */ ++ true_reg->min_value = 0; ++ } + /* If this is false then constant < register, if it is true then + * the register < constant. + */ + false_reg->min_value = val + 1; ++ false_reg->value_from_signed = value_from_signed; + true_reg->max_value = val; ++ true_reg->value_from_signed = value_from_signed; + break; + default: + break; +@@ -1943,6 +2019,12 @@ static void reg_set_min_max_inv(struct bpf_reg_state *true_reg, + + check_reg_overflow(false_reg); + check_reg_overflow(true_reg); ++ if (is_range) { ++ if (__is_pointer_value(false, false_reg)) ++ reset_reg_range_values(false_reg, 0); ++ if (__is_pointer_value(false, true_reg)) ++ reset_reg_range_values(true_reg, 0); ++ } + } + + static void mark_map_reg(struct bpf_reg_state *regs, u32 regno, u32 id, +-- +2.7.4 + |