Oops, gmail seems to have mangled everything. Will resend using git send-email.
I've added the test cases for mov64, but I'm not sure of the expected mov32 behavior. Currently coerce_reg_to_size() is called after mark_reg_unknown(), which sets the bounds to 64bits. coerce_reg_to_size() resets the bounds again, as they're too "wide" to fit the new size. It sets SMIN = UMIN = 0, which seems weird. Shouldn't SMIN be 1 << (size * 8 - 1)? Same applies for SMAX. Should mov32 always mark the dst as unbounded? On Thu, Jul 26, 2018 at 1:42 AM, Daniel Borkmann <dan...@iogearbox.net> wrote: > On 07/26/2018 12:08 AM, Arthur Fabre 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); > > if (err) > > return err; > > > > Thanks a lot for the patch! Looks like it's corrupted wrt newline. > > Please also add test cases to tools/testing/selftests/bpf/test_verifier.c > for the cases of mov64 and mov32 where in each src==dst and src!=dst; mov32 > should mark it as unbounded but not former, so would be good to keep > tracking > that in selftests. >