netdev - Re: [PATCH v2 1/2] bpf: Forbid XADD on spilled pointers for unprivileged users

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: <CAADnVQ+0U5K_ySgHcM-o6mbq-mcntA4XrRJe9QVHc0fUj2f2Dg@mail.gmail.com>
Date:   Mon, 20 Apr 2020 18:48:54 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] bpf: Forbid XADD on spilled pointers for
 unprivileged users

On Thu, Apr 16, 2020 at 5:00 PM Jann Horn <jannh@...gle.com> wrote:
>
> When check_xadd() verifies an XADD operation on a pointer to a stack slot
> containing a spilled pointer, check_stack_read() verifies that the read,
> which is part of XADD, is valid. However, since the placeholder value -1 is
> passed as `value_regno`, check_stack_read() can only return a binary
> decision and can't return the type of the value that was read. The intent
> here is to verify whether the value read from the stack slot may be used as
> a SCALAR_VALUE; but since check_stack_read() doesn't check the type, and
> the type information is lost when check_stack_read() returns, this is not
> enforced, and a malicious user can abuse XADD to leak spilled kernel
> pointers.
>
> Fix it by letting check_stack_read() verify that the value is usable as a
> SCALAR_VALUE if no type information is passed to the caller.
>
> To be able to use __is_pointer_value() in check_stack_read(), move it up.
>
> Fix up the expected unprivileged error message for a BPF selftest that,
> until now, assumed that unprivileged users can use XADD on stack-spilled
> pointers. This also gives us a test for the behavior introduced in this
> patch for free.
>
> In theory, this could also be fixed by forbidding XADD on stack spills
> entirely, since XADD is a locked operation (for operations on memory with
> concurrency) and there can't be any concurrency on the BPF stack; but
> Alexei has said that he wants to keep XADD on stack slots working to avoid
> changes to the test suite [1].
>
> The following BPF program demonstrates how to leak a BPF map pointer as an
> unprivileged user using this bug:
>
>     // r7 = map_pointer
>     BPF_LD_MAP_FD(BPF_REG_7, small_map),
>     // r8 = launder(map_pointer)
>     BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_7, -8),
>     BPF_MOV64_IMM(BPF_REG_1, 0),
>     ((struct bpf_insn) {
>       .code  = BPF_STX | BPF_DW | BPF_XADD,
>       .dst_reg = BPF_REG_FP,
>       .src_reg = BPF_REG_1,
>       .off = -8
>     }),
>     BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_FP, -8),
>
>     // store r8 into map
>     BPF_MOV64_REG(BPF_REG_ARG1, BPF_REG_7),
>     BPF_MOV64_REG(BPF_REG_ARG2, BPF_REG_FP),
>     BPF_ALU64_IMM(BPF_ADD, BPF_REG_ARG2, -4),
>     BPF_ST_MEM(BPF_W, BPF_REG_ARG2, 0, 0),
>     BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
>     BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
>     BPF_EXIT_INSN(),
>     BPF_STX_MEM(BPF_DW, BPF_REG_0, BPF_REG_8, 0),
>
>     BPF_MOV64_IMM(BPF_REG_0, 0),
>     BPF_EXIT_INSN()
>
> [1] https://lore.kernel.org/bpf/20200416211116.qxqcza5vo2ddnkdq@ast-mbp.dhcp.thefacebook.com/
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Jann Horn <jannh@...gle.com>

Applied both. Thanks