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: <64b39825-3f74-15f6-6212-057c96a84999@iogearbox.net>
Date:   Mon, 30 Jul 2018 14:03:07 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     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 07/30/2018 12:58 PM, Arthur Fabre wrote:
> On Mon, Jul 30, 2018 at 10:10 AM, Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 07/30/2018 09:44 AM, Arthur Fabre wrote:
>>> On Sun, Jul 29, 2018 at 4:59 PM, Alexei Starovoitov
>>> <alexei.starovoitov@...il.com> wrote:
>>>> On Thu, Jul 26, 2018 at 1:08 AM, Arthur Fabre <afabre@...udflare.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 <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);
>>>>
>>>> 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.

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.