lkml.org 
[lkml]   [2021]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.15 013/177] bpf: Fix kernel address leakage in atomic fetch
    Date
    From: Daniel Borkmann <daniel@iogearbox.net>

    commit 7d3baf0afa3aa9102d6a521a8e4c41888bb79882 upstream.

    The change in commit 37086bfdc737 ("bpf: Propagate stack bounds to registers
    in atomics w/ BPF_FETCH") around check_mem_access() handling is buggy since
    this would allow for unprivileged users to leak kernel pointers. For example,
    an atomic fetch/and with -1 on a stack destination which holds a spilled
    pointer will migrate the spilled register type into a scalar, which can then
    be exported out of the program (since scalar != pointer) by dumping it into
    a map value.

    The original implementation of XADD was preventing this situation by using
    a double call to check_mem_access() one with BPF_READ and a subsequent one
    with BPF_WRITE, in both cases passing -1 as a placeholder value instead of
    register as per XADD semantics since it didn't contain a value fetch. The
    BPF_READ also included a check in check_stack_read_fixed_off() which rejects
    the program if the stack slot is of __is_pointer_value() if dst_regno < 0.
    The latter is to distinguish whether we're dealing with a regular stack spill/
    fill or some arithmetical operation which is disallowed on non-scalars, see
    also 6e7e63cbb023 ("bpf: Forbid XADD on spilled pointers for unprivileged
    users") for more context on check_mem_access() and its handling of placeholder
    value -1.

    One minimally intrusive option to fix the leak is for the BPF_FETCH case to
    initially check the BPF_READ case via check_mem_access() with -1 as register,
    followed by the actual load case with non-negative load_reg to propagate
    stack bounds to registers.

    Fixes: 37086bfdc737 ("bpf: Propagate stack bounds to registers in atomics w/ BPF_FETCH")
    Reported-by: <n4ke4mry@gmail.com>
    Acked-by: Brendan Jackman <jackmanb@google.com>
    Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
    Signed-off-by: Alexei Starovoitov <ast@kernel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    kernel/bpf/verifier.c | 12 +++++++++---
    1 file changed, 9 insertions(+), 3 deletions(-)

    --- a/kernel/bpf/verifier.c
    +++ b/kernel/bpf/verifier.c
    @@ -4417,13 +4417,19 @@ static int check_atomic(struct bpf_verif
    load_reg = -1;
    }

    - /* check whether we can read the memory */
    + /* Check whether we can read the memory, with second call for fetch
    + * case to simulate the register fill.
    + */
    err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
    - BPF_SIZE(insn->code), BPF_READ, load_reg, true);
    + BPF_SIZE(insn->code), BPF_READ, -1, true);
    + if (!err && load_reg >= 0)
    + err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
    + BPF_SIZE(insn->code), BPF_READ, load_reg,
    + true);
    if (err)
    return err;

    - /* check whether we can write into the same memory */
    + /* Check whether we can write into the same memory. */
    err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
    BPF_SIZE(insn->code), BPF_WRITE, -1, true);
    if (err)

    \
     
     \ /
      Last update: 2021-12-20 16:04    [W:4.094 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site