From: Kurt Manucredo <fuzzybritches0@gmail.com>
To: ebiggers@kernel.org,
syzbot+bed360704c521841c85d@syzkaller.appspotmail.com
Cc: Kurt Manucredo <fuzzybritches0@gmail.com>,
keescook@chromium.org, yhs@fb.com, dvyukov@google.com,
andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
daniel@iogearbox.net, davem@davemloft.net, hawk@kernel.org,
john.fastabend@gmail.com, kafai@fb.com, kpsingh@kernel.org,
kuba@kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, songliubraving@fb.com,
syzkaller-bugs@googlegroups.com, nathan@kernel.org,
ndesaulniers@google.com, clang-built-linux@googlegroups.com,
kernel-hardening@lists.openwall.com, kasan-dev@googlegroups.com
Subject: [PATCH v5] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run
Date: Tue, 15 Jun 2021 16:42:10 +0000 [thread overview]
Message-ID: <85536-177443-curtm@phaethon> (raw)
In-Reply-To: <YMJvbGEz0xu9JU9D@gmail.com>
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@syzkaller.appspotmail.com
Signed-off-by: Kurt Manucredo <fuzzybritches0@gmail.com>
---
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
next prev parent reply other threads:[~2021-06-15 16:42 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <000000000000c2987605be907e41@google.com>
[not found] ` <20210602212726.7-1-fuzzybritches0@gmail.com>
[not found] ` <YLhd8BL3HGItbXmx@kroah.com>
[not found] ` <87609-531187-curtm@phaethon>
[not found] ` <6a392b66-6f26-4532-d25f-6b09770ce366@fb.com>
[not found] ` <CAADnVQKexxZQw0yK_7rmFOdaYabaFpi2EmF6RGs5bXvFHtUQaA@mail.gmail.com>
2021-06-07 7:38 ` [PATCH v4] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run Dmitry Vyukov
2021-06-09 18:20 ` Kees Cook
2021-06-09 23:40 ` Yonghong Song
2021-06-10 5:32 ` Dmitry Vyukov
2021-06-10 6:06 ` Yonghong Song
2021-06-10 17:06 ` Kees Cook
2021-06-10 17:52 ` Alexei Starovoitov
2021-06-10 20:00 ` Eric Biggers
2021-06-15 16:42 ` Kurt Manucredo [this message]
2021-06-15 18:51 ` [PATCH v5] " Edward Cree
2021-06-15 19:33 ` Eric Biggers
2021-06-15 21:08 ` Daniel Borkmann
2021-06-15 21:32 ` Eric Biggers
2021-06-15 21:38 ` Eric Biggers
2021-06-15 21:54 ` Daniel Borkmann
2021-06-15 22:07 ` Eric Biggers
2021-06-15 22:31 ` Kurt Manucredo
2021-06-17 10:09 ` Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85536-177443-curtm@phaethon \
--to=fuzzybritches0@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=clang-built-linux@googlegroups.com \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=dvyukov@google.com \
--cc=ebiggers@kernel.org \
--cc=hawk@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kasan-dev@googlegroups.com \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=syzbot+bed360704c521841c85d@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).