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] [day] [month] [year] [list]

Message-ID: <d3ec24aa-c655-d14b-0dd1-720118adabc4@solarflare.com>
Date:   Mon, 30 Jul 2018 17:40:53 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Arthur Fabre <afabre@...udflare.com>
CC:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] bpf: verifier: BPF_MOV don't mark dst reg if src == dst

On 30/07/18 13:03, Daniel Borkmann wrote:
> On 07/30/2018 12:58 PM, Arthur Fabre wrote:
>> 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.
I guess the right thing is to change the existing call to DST_OP_NO_MARK,
 and then in the "case: R = imm" branch add another check_reg_arg(DST_OP)
 (or maybe just a mark_reg_unknown()).

-Ed