Syzbot detects a shift-out-of-bounds in ___bpf_prog_run() kernel/bpf/core.c:1414:2. The shift-out-of-bounds happens when we have BPF_X. This means we have to go the same way we go when we want to avoid a divide-by-zero. We do it in do_misc_fixups(). When we have BPF_K we find divide-by-zero and shift-out-of-bounds guards next each other in check_alu_op(). It seems only logical to me that the same should be true for BPF_X in do_misc_fixups() since it is there where I found the divide-by-zero guard. Or is there a reason I'm not aware of, that dictates that the checks should be in adjust_scalar_min_max_vals(), as they are now? This patch was tested by syzbot. Reported-and-tested-by: syzbot+bed360704c521841c85d@xxxxxxxxxxxxxxxxxxxxxxxxx Signed-off-by: Kurt Manucredo <fuzzybritches0@xxxxxxxxx> --- https://syzkaller.appspot.com/bug?id=edb51be4c9a320186328893287bb30d5eed09231 Changelog: ---------- v5 - Fix shift-out-of-bounds in do_misc_fixups(). v4 - Fix shift-out-of-bounds in adjust_scalar_min_max_vals. Fix commit message. v3 - Make it clearer what the fix is for. v2 - Fix shift-out-of-bounds in ___bpf_prog_run() by adding boundary check in check_alu_op() in verifier.c. v1 - Fix shift-out-of-bounds in ___bpf_prog_run() by adding boundary check in ___bpf_prog_run(). thanks kind regards Kurt kernel/bpf/verifier.c | 53 +++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 94ba5163d4c5..83c7c1ccaf26 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7496,7 +7496,6 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, u64 umin_val, umax_val; s32 s32_min_val, s32_max_val; u32 u32_min_val, u32_max_val; - u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32; bool alu32 = (BPF_CLASS(insn->code) != BPF_ALU64); int ret; @@ -7592,39 +7591,18 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, scalar_min_max_xor(dst_reg, &src_reg); break; case BPF_LSH: - if (umax_val >= insn_bitness) { - /* Shifts greater than 31 or 63 are undefined. - * This includes shifts by a negative number. - */ - mark_reg_unknown(env, regs, insn->dst_reg); - break; - } if (alu32) scalar32_min_max_lsh(dst_reg, &src_reg); else scalar_min_max_lsh(dst_reg, &src_reg); break; case BPF_RSH: - if (umax_val >= insn_bitness) { - /* Shifts greater than 31 or 63 are undefined. - * This includes shifts by a negative number. - */ - mark_reg_unknown(env, regs, insn->dst_reg); - break; - } if (alu32) scalar32_min_max_rsh(dst_reg, &src_reg); else scalar_min_max_rsh(dst_reg, &src_reg); break; case BPF_ARSH: - if (umax_val >= insn_bitness) { - /* Shifts greater than 31 or 63 are undefined. - * This includes shifts by a negative number. - */ - mark_reg_unknown(env, regs, insn->dst_reg); - break; - } if (alu32) scalar32_min_max_arsh(dst_reg, &src_reg); else @@ -12353,6 +12331,37 @@ static int do_misc_fixups(struct bpf_verifier_env *env) continue; } + /* Make shift-out-of-bounds exceptions impossible. */ + if (insn->code == (BPF_ALU64 | BPF_LSH | BPF_X) || + insn->code == (BPF_ALU64 | BPF_RSH | BPF_X) || + insn->code == (BPF_ALU64 | BPF_ARSH | BPF_X) || + insn->code == (BPF_ALU | BPF_LSH | BPF_X) || + insn->code == (BPF_ALU | BPF_RSH | BPF_X) || + insn->code == (BPF_ALU | BPF_ARSH | BPF_X)) { + bool is64 = BPF_CLASS(insn->code) == BPF_ALU64; + u8 insn_bitness = is64 ? 64 : 32; + struct bpf_insn chk_and_shift[] = { + /* [R,W]x shift >= 32||64 -> 0 */ + BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) | + BPF_JLT | BPF_K, insn->src_reg, + insn_bitness, 2, 0), + BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg), + BPF_JMP_IMM(BPF_JA, 0, 0, 1), + *insn, + }; + + cnt = ARRAY_SIZE(chk_and_shift); + + new_prog = bpf_patch_insn_data(env, i + delta, chk_and_shift, cnt); + if (!new_prog) + return -ENOMEM; + + delta += cnt - 1; + env->prog = prog = new_prog; + insn = new_prog->insnsi + i + delta; + continue; + } + /* Implement LD_ABS and LD_IND with a rewrite, if supported by the program type. */ if (BPF_CLASS(insn->code) == BPF_LD && (BPF_MODE(insn->code) == BPF_ABS || -- 2.30.2
