bpf: Fix leakage of uninitialized bpf stack under speculation

commit 801c6058d14a82179a7ee17a4b532cac6fad067f upstream.

The current implemented mechanisms to mitigate data disclosure under
speculation mainly address stack and map value oob access from the
speculative domain. However, Piotr discovered that uninitialized BPF
stack is not protected yet, and thus old data from the kernel stack,
potentially including addresses of kernel structures, could still be
extracted from that 512 bytes large window. The BPF stack is special
compared to map values since it's not zero initialized for every
program invocation, whereas map values /are/ zero initialized upon
their initial allocation and thus cannot leak any prior data in either
domain. In the non-speculative domain, the verifier ensures that every
stack slot read must have a prior stack slot write by the BPF program
to avoid such data leaking issue.

However, this is not enough: for example, when the pointer arithmetic
operation moves the stack pointer from the last valid stack offset to
the first valid offset, the sanitation logic allows for any intermediate
offsets during speculative execution, which could then be used to
extract any restricted stack content via side-channel.

Given for unprivileged stack pointer arithmetic the use of unknown
but bounded scalars is generally forbidden, we can simply turn the
register-based arithmetic operation into an immediate-based arithmetic
operation without the need for masking. This also gives the benefit
of reducing the needed instructions for the operation. Given after
the work in 7fedb63a8307 ("bpf: Tighten speculative pointer arithmetic
mask"), the aux->alu_limit already holds the final immediate value for
the offset register with the known scalar. Thus, a simple mov of the
immediate to AX register with using AX as the source for the original
instruction is sufficient and possible now in this case.

Reported-by: Piotr Krysiuk <piotras@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Tested-by: Piotr Krysiuk <piotras@gmail.com>
Reviewed-by: Piotr Krysiuk <piotras@gmail.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
[fllinden@amazon.com: fixed minor 4.14 conflict because of renamed function]
Signed-off-by: Frank van der Linden <fllinden@amazon.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index d8b3240cfe6eed50fab4521eba48bdf5e000871e..8509484cada4a9e88dd806d10ccce817adb6c280 100644 (file)
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -114,10 +114,11 @@ struct bpf_verifier_state_list {
 };
 
 /* Possible states for alu_state member. */
-#define BPF_ALU_SANITIZE_SRC           1U
-#define BPF_ALU_SANITIZE_DST           2U
+#define BPF_ALU_SANITIZE_SRC           (1U << 0)
+#define BPF_ALU_SANITIZE_DST           (1U << 1)
 #define BPF_ALU_NEG_VALUE              (1U << 2)
 #define BPF_ALU_NON_POINTER            (1U << 3)
+#define BPF_ALU_IMMEDIATE              (1U << 4)
 #define BPF_ALU_SANITIZE               (BPF_ALU_SANITIZE_SRC | \
                                         BPF_ALU_SANITIZE_DST)
 

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5c440398e0a170fa3fba7bc8416c16ac44ed7687..84779fe35ebc87fe9621a2d46b506d75a9892743 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2120,6 +2120,7 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env,
 {
        struct bpf_insn_aux_data *aux = commit_window ? cur_aux(env) : tmp_aux;
        struct bpf_verifier_state *vstate = env->cur_state;
+       bool off_is_imm = tnum_is_const(off_reg->var_off);
        bool off_is_neg = off_reg->smin_value < 0;
        bool ptr_is_dst_reg = ptr_reg == dst_reg;
        u8 opcode = BPF_OP(insn->code);
@@ -2150,6 +2151,7 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env,
                alu_limit = abs(tmp_aux->alu_limit - alu_limit);
        } else {
                alu_state  = off_is_neg ? BPF_ALU_NEG_VALUE : 0;
+               alu_state |= off_is_imm ? BPF_ALU_IMMEDIATE : 0;
                alu_state |= ptr_is_dst_reg ?
                             BPF_ALU_SANITIZE_SRC : BPF_ALU_SANITIZE_DST;
        }
@@ -4850,7 +4852,7 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
                        const u8 code_sub = BPF_ALU64 | BPF_SUB | BPF_X;
                        struct bpf_insn insn_buf[16];
                        struct bpf_insn *patch = &insn_buf[0];
-                       bool issrc, isneg;
+                       bool issrc, isneg, isimm;
                        u32 off_reg;
 
                        aux = &env->insn_aux_data[i + delta];
@@ -4861,16 +4863,21 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
                        isneg = aux->alu_state & BPF_ALU_NEG_VALUE;
                        issrc = (aux->alu_state & BPF_ALU_SANITIZE) ==
                                BPF_ALU_SANITIZE_SRC;
+                       isimm = aux->alu_state & BPF_ALU_IMMEDIATE;
 
                        off_reg = issrc ? insn->src_reg : insn->dst_reg;
-                       if (isneg)
-                               *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1);
-                       *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit);
-                       *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg);
-                       *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg);
-                       *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0);
-                       *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63);
-                       *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, off_reg);
+                       if (isimm) {
+                               *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit);
+                       } else {
+                               if (isneg)
+                                       *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1);
+                               *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit);
+                               *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg);
+                               *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg);
+                               *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0);
+                               *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63);
+                               *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, off_reg);
+                       }
                        if (!issrc)
                                *patch++ = BPF_MOV64_REG(insn->dst_reg, insn->src_reg);
                        insn->src_reg = BPF_REG_AX;
@@ -4878,7 +4885,7 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
                                insn->code = insn->code == code_add ?
                                             code_sub : code_add;
                        *patch++ = *insn;
-                       if (issrc && isneg)
+                       if (issrc && isneg && !isimm)
                                *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1);
                        cnt = patch - insn_buf;