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]

Date:   Mon, 30 Jul 2018 11:58:34 +0100
From:   Arthur Fabre <afabre@...udflare.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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 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.