netdev - Re: [PATCH] bpf: verifier: BPF_MOV don't mark dst reg if src == dst

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOn4ftsOedWzGNRHb2RhkADJh-qBFNLPoH89MUsZxxZri52_Tg@mail.gmail.com>
Date:   Thu, 26 Jul 2018 17:52:18 +0100
From:   Arthur Fabre <afabre@...udflare.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH] bpf: verifier: BPF_MOV don't mark dst reg if src == dst

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 <daniel@...earbox.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 <afabre@...udflare.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.
>

Content of type "text/html" skipped