From b1de2653e42998b582f124ff15adfd908b5185f1 Mon Sep 17 00:00:00 2001 From: Shile Zhang Date: Tue, 18 Jan 2022 19:52:55 +0800 Subject: [PATCH] Revert "bpf: Fix leakage due to insufficient speculative store bypass mitigation" ANBZ: #342 This reverts commit 6e458fd75eb655cbe0aa42f04be4dca39a03154b. Signed-off-by: Qiao Ma Acked-by: Mao Wenan Acked-by: Tony Lu --- include/linux/bpf_verifier.h | 2 +- kernel/bpf/verifier.c | 88 ++++++++++++++++++++++-------------- 2 files changed, 56 insertions(+), 34 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index e64ac93f7f4c0d..daab0960c05442 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -158,8 +158,8 @@ struct bpf_insn_aux_data { u32 alu_limit; /* limit for add/sub register with pointer */ }; int ctx_field_size; /* the ctx field size for load insn, maybe 0 */ + int sanitize_stack_off; /* stack slot to be cleared */ bool seen; /* this insn was processed by the verifier */ - bool sanitize_stack_spill; /* subject to Spectre v4 sanitation */ u8 alu_state; /* used in combination with alu_limit */ }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6096faffb3dd69..f4d273fdb30ea4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1009,19 +1009,6 @@ static int check_stack_write(struct bpf_verifier_env *env, cur = env->cur_state->frame[env->cur_state->curframe]; if (value_regno >= 0) reg = &cur->regs[value_regno]; - if (!env->allow_ptr_leaks) { - bool sanitize = reg && is_spillable_regtype(reg->type); - - for (i = 0; i < size; i++) { - if (state->stack[spi].slot_type[i] == STACK_INVALID) { - sanitize = true; - break; - } - } - - if (sanitize) - env->insn_aux_data[insn_idx].sanitize_stack_spill = true; - } if (reg && size == BPF_REG_SIZE && register_is_const(reg) && !register_is_null(reg) && env->allow_ptr_leaks) { @@ -1032,10 +1019,47 @@ static int check_stack_write(struct bpf_verifier_env *env, verbose(env, "invalid size of register spill\n"); return -EACCES; } + if (state != cur && reg->type == PTR_TO_STACK) { verbose(env, "cannot spill pointers to stack into stack frame of the caller\n"); return -EINVAL; } + + if (!env->allow_ptr_leaks) { + bool sanitize = false; + + if (state->stack[spi].slot_type[0] == STACK_SPILL && + register_is_const(&state->stack[spi].spilled_ptr)) + sanitize = true; + for (i = 0; i < BPF_REG_SIZE; i++) + if (state->stack[spi].slot_type[i] == STACK_MISC) { + sanitize = true; + break; + } + if (sanitize) { + int *poff = &env->insn_aux_data[insn_idx].sanitize_stack_off; + int soff = (-spi - 1) * BPF_REG_SIZE; + + /* detected reuse of integer stack slot with a pointer + * which means either llvm is reusing stack slot or + * an attacker is trying to exploit CVE-2018-3639 + * (speculative store bypass) + * Have to sanitize that slot with preemptive + * store of zero. + */ + if (*poff && *poff != soff) { + /* disallow programs where single insn stores + * into two different stack slots, since verifier + * cannot sanitize them + */ + verbose(env, + "insn %d cannot access two stack slots fp%d and fp%d", + insn_idx, *poff, soff); + return -EINVAL; + } + *poff = soff; + } + } save_register_state(state, spi, reg); } else { u8 type = STACK_MISC; @@ -5808,33 +5832,34 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) insn = env->prog->insnsi + delta; for (i = 0; i < insn_cnt; i++, insn++) { - bool ctx_access; - if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) || insn->code == (BPF_LDX | BPF_MEM | BPF_H) || insn->code == (BPF_LDX | BPF_MEM | BPF_W) || - insn->code == (BPF_LDX | BPF_MEM | BPF_DW)) { + insn->code == (BPF_LDX | BPF_MEM | BPF_DW)) type = BPF_READ; - ctx_access = true; - } else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) || - insn->code == (BPF_STX | BPF_MEM | BPF_H) || - insn->code == (BPF_STX | BPF_MEM | BPF_W) || - insn->code == (BPF_STX | BPF_MEM | BPF_DW) || - insn->code == (BPF_ST | BPF_MEM | BPF_B) || - insn->code == (BPF_ST | BPF_MEM | BPF_H) || - insn->code == (BPF_ST | BPF_MEM | BPF_W) || - insn->code == (BPF_ST | BPF_MEM | BPF_DW)) { + else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) || + insn->code == (BPF_STX | BPF_MEM | BPF_H) || + insn->code == (BPF_STX | BPF_MEM | BPF_W) || + insn->code == (BPF_STX | BPF_MEM | BPF_DW)) type = BPF_WRITE; - ctx_access = BPF_CLASS(insn->code) == BPF_STX; - } else { + else continue; - } if (type == BPF_WRITE && - env->insn_aux_data[i + delta].sanitize_stack_spill) { + env->insn_aux_data[i + delta].sanitize_stack_off) { struct bpf_insn patch[] = { + /* Sanitize suspicious stack slot with zero. + * There are no memory dependencies for this store, + * since it's only using frame pointer and immediate + * constant of zero + */ + BPF_ST_MEM(BPF_DW, BPF_REG_FP, + env->insn_aux_data[i + delta].sanitize_stack_off, + 0), + /* the original STX instruction will immediately + * overwrite the same stack slot with appropriate value + */ *insn, - BPF_ST_NOSPEC(), }; cnt = ARRAY_SIZE(patch); @@ -5848,9 +5873,6 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) continue; } - if (!ctx_access) - continue; - if (env->insn_aux_data[i + delta].ptr_type != PTR_TO_CTX) continue;