| Message ID | 20231121002221.3687787-7-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Complete BPF verifier precision tracking support for register spills | expand |
On Mon, 2023-11-20 at 16:22 -0800, Andrii Nakryiko wrote: > Similar to special handling of STACK_ZERO, when reading 1/2/4 bytes from > stack from slot that has register spilled into it and that register has > a constant value zero, preserve that zero and mark spilled register as > precise for that. This makes spilled const zero register and STACK_ZERO > cases equivalent in their behavior. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- Acked-by: Eduard Zingerman <eddyz87@gmail.com> [...] > + if (spill_cnt == size && > + tnum_is_const(reg->var_off) && reg->var_off.value == 0) { > + __mark_reg_const_zero(&state->regs[dst_regno]); > + /* this IS register fill, so keep insn_flags */ > + } else if (zero_cnt == size) { > + /* similarly to mark_reg_stack_read(), preserve zeroes */ > + __mark_reg_const_zero(&state->regs[dst_regno]); > + insn_flags = 0; /* not restoring original register state */ nit: In case if there would be v3, could you please leave a comment here, something like below: when check_stack_write_fixed_off() puts STACK_ZERO marks for writes, not aligned on register boundary, it marks source registers precise. Thus, additional precision propagation is necessary in this case and insn_flags could be cleared. or something along these lines? > + } else { > + mark_reg_unknown(env, state->regs, dst_regno); > + insn_flags = 0; /* not restoring original register state */ > + } > } > state->regs[dst_regno].live |= REG_LIVE_WRITTEN; > } else if (dst_regno >= 0) {
On Tue, Nov 21, 2023 at 8:20 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2023-11-20 at 16:22 -0800, Andrii Nakryiko wrote: > > Similar to special handling of STACK_ZERO, when reading 1/2/4 bytes from > > stack from slot that has register spilled into it and that register has > > a constant value zero, preserve that zero and mark spilled register as > > precise for that. This makes spilled const zero register and STACK_ZERO > > cases equivalent in their behavior. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > Acked-by: Eduard Zingerman <eddyz87@gmail.com> > > [...] > > > + if (spill_cnt == size && > > + tnum_is_const(reg->var_off) && reg->var_off.value == 0) { > > + __mark_reg_const_zero(&state->regs[dst_regno]); > > + /* this IS register fill, so keep insn_flags */ > > + } else if (zero_cnt == size) { > > + /* similarly to mark_reg_stack_read(), preserve zeroes */ > > + __mark_reg_const_zero(&state->regs[dst_regno]); > > + insn_flags = 0; /* not restoring original register state */ > > nit: In case if there would be v3, could you please > leave a comment here, something like below: > > when check_stack_write_fixed_off() puts STACK_ZERO marks > for writes, not aligned on register boundary, it marks source > registers precise. Thus, additional precision propagation is > necessary in this case and insn_flags could be cleared. > > or something along these lines? > Hm... this seems misleading, precision propagation of original register on write is orthogonal to this. The real reason why we clear insn_flags is because there is no *register* fill here. There might not be any spilled register at all here, or a completely irrelevant spilled register. This instruction is basically `r1 = 0;`, though obfuscated as stack dereference. So from the backtracking side of things, there is no stack access here. > > + } else { > > + mark_reg_unknown(env, state->regs, dst_regno); > > + insn_flags = 0; /* not restoring original register state */ > > + } > > } > > state->regs[dst_regno].live |= REG_LIVE_WRITTEN; > > } else if (dst_regno >= 0) { > > >
On Tue, 2023-11-21 at 10:14 -0800, Andrii Nakryiko wrote: > On Tue, Nov 21, 2023 at 8:20 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Mon, 2023-11-20 at 16:22 -0800, Andrii Nakryiko wrote: > > > Similar to special handling of STACK_ZERO, when reading 1/2/4 bytes from > > > stack from slot that has register spilled into it and that register has > > > a constant value zero, preserve that zero and mark spilled register as > > > precise for that. This makes spilled const zero register and STACK_ZERO > > > cases equivalent in their behavior. > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > > Acked-by: Eduard Zingerman <eddyz87@gmail.com> > > > > [...] > > > > > + if (spill_cnt == size && > > > + tnum_is_const(reg->var_off) && reg->var_off.value == 0) { > > > + __mark_reg_const_zero(&state->regs[dst_regno]); > > > + /* this IS register fill, so keep insn_flags */ > > > + } else if (zero_cnt == size) { > > > + /* similarly to mark_reg_stack_read(), preserve zeroes */ > > > + __mark_reg_const_zero(&state->regs[dst_regno]); > > > + insn_flags = 0; /* not restoring original register state */ > > > > nit: In case if there would be v3, could you please > > leave a comment here, something like below: > > > > when check_stack_write_fixed_off() puts STACK_ZERO marks > > for writes, not aligned on register boundary, it marks source > > registers precise. Thus, additional precision propagation is > > necessary in this case and insn_flags could be cleared. > > > > or something along these lines? > > > > Hm... this seems misleading, precision propagation of original > register on write is orthogonal to this. The real reason why we clear > insn_flags is because there is no *register* fill here. There might > not be any spilled register at all here, or a completely irrelevant > spilled register. This instruction is basically `r1 = 0;`, though > obfuscated as stack dereference. So from the backtracking side of > things, there is no stack access here. Hm, I mostly agree. This comment would be relevant only before patch #8 in this series.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 5145afb5da25..00e6b17b71d0 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4695,22 +4695,39 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, copy_register_state(&state->regs[dst_regno], reg); state->regs[dst_regno].subreg_def = subreg_def; } else { + int spill_cnt = 0, zero_cnt = 0; + for (i = 0; i < size; i++) { type = stype[(slot - i) % BPF_REG_SIZE]; - if (type == STACK_SPILL) + if (type == STACK_SPILL) { + spill_cnt++; continue; + } if (type == STACK_MISC) continue; - if (type == STACK_ZERO) + if (type == STACK_ZERO) { + zero_cnt++; continue; + } if (type == STACK_INVALID && env->allow_uninit_stack) continue; verbose(env, "invalid read from stack off %d+%d size %d\n", off, i, size); return -EACCES; } - mark_reg_unknown(env, state->regs, dst_regno); - insn_flags = 0; /* not restoring original register state */ + + if (spill_cnt == size && + tnum_is_const(reg->var_off) && reg->var_off.value == 0) { + __mark_reg_const_zero(&state->regs[dst_regno]); + /* this IS register fill, so keep insn_flags */ + } else if (zero_cnt == size) { + /* similarly to mark_reg_stack_read(), preserve zeroes */ + __mark_reg_const_zero(&state->regs[dst_regno]); + insn_flags = 0; /* not restoring original register state */ + } else { + mark_reg_unknown(env, state->regs, dst_regno); + insn_flags = 0; /* not restoring original register state */ + } } state->regs[dst_regno].live |= REG_LIVE_WRITTEN; } else if (dst_regno >= 0) {
Similar to special handling of STACK_ZERO, when reading 1/2/4 bytes from stack from slot that has register spilled into it and that register has a constant value zero, preserve that zero and mark spilled register as precise for that. This makes spilled const zero register and STACK_ZERO cases equivalent in their behavior. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/bpf/verifier.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)