On 07/30/2018 12:58 PM, Arthur Fabre wrote: > On Mon, Jul 30, 2018 at 10:10 AM, Daniel Borkmann <dan...@iogearbox.net> > wrote: >> On 07/30/2018 09:44 AM, Arthur Fabre wrote: >>> On Sun, Jul 29, 2018 at 4:59 PM, Alexei Starovoitov >>> <alexei.starovoi...@gmail.com> wrote: >>>> On Thu, Jul 26, 2018 at 1:08 AM, Arthur Fabre <afa...@cloudflare.com> >>>> wrote: >>>>> When check_alu_op() handles a BPF_MOV between two registers, >>>>> it calls check_reg_arg() on the dst register, marking it as unbounded. >>>>> If the src and dst register are the same, this marks the src as >>>>> unbounded, which can lead to unexpected errors for further checks that >>>>> rely on bounds info. >>>>> >>>>> check_alu_op() now only marks the dst register as unbounded if it >>>>> different from the src register. >>>>> >>>>> Signed-off-by: Arthur Fabre <afa...@cloudflare.com> >>>>> --- >>>>> kernel/bpf/verifier.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>>>> index 63aaac52a265..ddfe3c544a80 100644 >>>>> --- a/kernel/bpf/verifier.c >>>>> +++ b/kernel/bpf/verifier.c >>>>> @@ -3238,8 +3238,9 @@ static int check_alu_op(struct bpf_verifier_env >>>>> *env, struct bpf_insn *insn) >>>>> } >>>>> } >>>>> >>>>> - /* check dest operand */ >>>>> - err = check_reg_arg(env, insn->dst_reg, DST_OP); >>>>> + /* check dest operand, only mark if dest != src */ >>>>> + err = check_reg_arg(env, insn->dst_reg, >>>>> + insn->dst_reg == insn->src_reg ? >>>>> DST_OP_NO_MARK : DST_OP); >>>> >>>> that doesn't look correct for 32-bit mov. >>>> Is that the case you're trying to improve? >>> >>> The patch was originally for 64-bit mov only >> >> Hmm, I'm not sure that is infact the case. The check_alu_op() is handled for >> 32 and 64 bit alu op case. So in the opcode == BPF_MOV case the >> check_reg_arg() >> on the dst register is done for both at that point, whereas retaining any >> current state should only be valid in 64 bit mov case, e.g. think of pointer >> types, these really need to be scratched here. I think it would make sense >> that >> after checking src operand we hold a temporary copy of its state and use that >> for setting regs[insn->dst_reg] later on under BPF_ALU64. > > The check_alu_op() call handles 32bit and 64bit cases, but then in the > 32bit case > mark_reg_unknown() is called, discarding all the dst register state. > I think this is equivalent to keeping a copy of dst and always marking > dst as unknown. > > I think we could actually always use check_reg_arg() with DST_OP_NO_MARK: > > In the 32bit case, we call mark_reg_unknown() anyways. > > In the 64bit case, we copy src to dst, so marking dst as unknown is pointless. > > For plain BPF, we call __mark_reg_known() anyways.
For imms this approach would be buggy since we leave a stale reg->off behind which is uncleared from previous reg state. So for them the mark_reg_unknown() is useful in the sense that it clears all reg state whereas __mark_reg_known() might only initialize a subset of it.
