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: <CAH3MdRUqSkjvFeQRAn+Bb2FNmNukcD8_4SrMi6ahw=f26ky0+A@mail.gmail.com>
Date:   Thu, 26 Jul 2018 16:21:28 -0700
From:   Y Song <ys114321@...il.com>
To:     Arthur Fabre <afabre@...udflare.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] bpf: verifier: BPF_MOV don't mark dst reg if src == dst

On Thu, Jul 26, 2018 at 9:52 AM, Arthur Fabre <afabre@...udflare.com> wrote:
> 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.

The interpreter has below:

        ALU_MOV_X:
                DST = (u32) SRC;
                CONT;
        ...
        ALU64_MOV_X:
                DST = SRC;
                CONT;

The later verifier code does seem to mark dst_reg properly for both
ALU and ALU64.

> 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.

The SMIN/UMIN still should be 0 since there is no negative here due to
smaller width?

> Should mov32 always mark the dst as unbounded?

We can do better than unbounded for dst register of mov32, which is
the code already
doing?

>
>
> 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.

Could you explain (and add to the commit messages eventually) what
are these unexpected errors?

>> >
>> > 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.
>
>