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: <7b0ce11d-ad54-9ba8-740c-58fd2675cc69@iogearbox.net>
Date:   Mon, 30 Jul 2018 11:10:25 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Arthur Fabre <afabre@...udflare.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     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 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.