linux-kernel - Re: [PATCH v1] bpf: Set register type according to is_valid

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]

Date:   Thu, 22 Sep 2016 21:53:06 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Daniel Borkmann <daniel@...earbox.net>,
        linux-kernel@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Kees Cook <keescook@...omium.org>,
        Sargun Dhillon <sargun@...gun.me>, Tejun Heo <tj@...nel.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH v1] bpf: Set register type according to is_valid_access()


On 22/09/2016 21:41, Daniel Borkmann wrote:
> On 09/22/2016 08:35 PM, Mickaël Salaün wrote:
>> This fix a pointer leak when an unprivileged eBPF program read a pointer
>> value from the context. Even if is_valid_access() returns a pointer
>> type, the eBPF verifier replace it with UNKNOWN_VALUE. The register
>> value containing an address is then allowed to leak. Moreover, this
>> prevented unprivileged eBPF programs to use functions with (legitimate)
>> pointer arguments.
>>
>> This bug is not an issue for now because the only unprivileged eBPF
>> program allowed is of type BPF_PROG_TYPE_SOCKET_FILTER and all the types
>> from its context are UNKNOWN_VALUE. However, this fix is important for
>> future unprivileged eBPF program types which could use pointers in their
>> context.
>>
>> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
>> Fixes: 969bf05eb3ce ("bpf: direct packet access")
>> Cc: Alexei Starovoitov <ast@...nel.org>
>> Cc: Andy Lutomirski <luto@...capital.net>
>> Cc: Daniel Borkmann <daniel@...earbox.net>
>> Cc: Kees Cook <keescook@...omium.org>
>> Acked-by: Sargun Dhillon <sargun@...gun.me>
>> ---
>>   kernel/bpf/verifier.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index daea765d72e6..0698ccd67715 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -794,10 +794,8 @@ static int check_mem_access(struct verifier_env
>> *env, u32 regno, int off,
>>           }
>>           err = check_ctx_access(env, off, size, t, &reg_type);
>>           if (!err && t == BPF_READ && value_regno >= 0) {
>> -            mark_reg_unknown_value(state->regs, value_regno);
>> -            if (env->allow_ptr_leaks)
>> -                /* note that reg.[id|off|range] == 0 */
>> -                state->regs[value_regno].type = reg_type;
>> +            /* note that reg.[id|off|range] == 0 */
>> +            state->regs[value_regno].type = reg_type;
> 
> True that it's not an issue currently, since reg_type is only set for
> PTR_TO_PACKET/PTR_TO_PACKET_END in xdp and tc programs that can only be
> loaded as privileged. So not an issue for BPF_PROG_TYPE_SOCKET_FILTER.
> 
> One thing I don't quite follow is why you remove the
> mark_reg_unknown_value()
> as this also clears imm? I think this could result in an actual verifier
> bug when it would reuse previous tracked imm value of that dst register?

Good catch, I missed the imm initialization. I'm going to send a new patch.

> 
>>           }
>>
>>       } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
>>
> 



Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)