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.
