From: "Kurt Manucredo" <fuzzybritches0@gmail.com> To: syzbot+bed360704c521841c85d@syzkaller.appspotmail.com, yhs@fb.com Cc: 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, linux-kernel-mentees@lists.linuxfoundation.org, skhan@linuxfoundation.org, gregkh@linuxfoundation.org Subject: Re: [PATCH v4] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run Date: Sun, 06 Jun 2021 21:15:46 +0200 [thread overview] Message-ID: <20484-14561-curtm@phaethon> (raw) In-Reply-To: <6a392b66-6f26-4532-d25f-6b09770ce366@fb.com> On Sat, 5 Jun 2021 10:55:25 -0700, Yonghong Song <yhs@fb.com> wrote: > > > > On 6/5/21 8:01 AM, Kurt Manucredo wrote: > > Syzbot detects a shift-out-of-bounds in ___bpf_prog_run() > > kernel/bpf/core.c:1414:2. > > This is not enough. We need more information on why this happens > so we can judge whether the patch indeed fixed the issue. > > > > > I propose: In adjust_scalar_min_max_vals() move boundary check up to avoid > > missing them and return with error when detected. > > > > 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: > > ---------- > > 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 | 30 +++++++++--------------------- > > 1 file changed, 9 insertions(+), 21 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 94ba5163d4c5..ed0eecf20de5 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7510,6 +7510,15 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, > > u32_min_val = src_reg.u32_min_value; > > u32_max_val = src_reg.u32_max_value; > > > > + if ((opcode == BPF_LSH || opcode == BPF_RSH || opcode == BPF_ARSH) && > > + umax_val >= insn_bitness) { > > + /* Shifts greater than 31 or 63 are undefined. > > + * This includes shifts by a negative number. > > + */ > > + verbose(env, "invalid shift %lldn", umax_val); > > + return -EINVAL; > > + } > > I think your fix is good. I would like to move after > the following code though: > > if (!src_known && > opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND) { > __mark_reg_unknown(env, dst_reg); > return 0; > } > It can only be right before that code not after. That's the latest. In the case of the syzbot bug, opcode == BPF_LSH and !src_known. Therefore it needs to be before that block of code. > > + > > if (alu32) { > > src_known = tnum_subreg_is_const(src_reg.var_off); > > if ((src_known && > > @@ -7592,39 +7601,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; > > - } > > I think this is what happens. For the above case, we simply > marks the dst reg as unknown and didn't fail verification. > So later on at runtime, the shift optimization will have wrong > shift value (> 31/64). Please correct me if this is not right > analysis. As I mentioned in the early please write detailed > analysis in commit log. > Shouldn't the src reg be changed so that the shift-out-of-bounds can't occur, if return -EINVAL is not what we want here? Changing the dst reg might not help. If I look into kernel/bpf/core.c I can see: DST = DST OP SRC; > Please also add a test at tools/testing/selftests/bpf/verifier/. > I'm going to look into selftests, kind regards thanks, Kurt Manucredo
WARNING: multiple messages have this Message-ID (diff)
From: "Kurt Manucredo" <fuzzybritches0@gmail.com> To: syzbot+bed360704c521841c85d@syzkaller.appspotmail.com, yhs@fb.com Cc: nathan@kernel.org, songliubraving@fb.com, kafai@fb.com, hawk@kernel.org, daniel@iogearbox.net, linux-kernel@vger.kernel.org, john.fastabend@gmail.com, andrii@kernel.org, ast@kernel.org, syzkaller-bugs@googlegroups.com, clang-built-linux@googlegroups.com, ndesaulniers@google.com, netdev@vger.kernel.org, kpsingh@kernel.org, kuba@kernel.org, bpf@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, davem@davemloft.net Subject: Re: [PATCH v4] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run Date: Sun, 06 Jun 2021 21:15:46 +0200 [thread overview] Message-ID: <20484-14561-curtm@phaethon> (raw) In-Reply-To: <6a392b66-6f26-4532-d25f-6b09770ce366@fb.com> On Sat, 5 Jun 2021 10:55:25 -0700, Yonghong Song <yhs@fb.com> wrote: > > > > On 6/5/21 8:01 AM, Kurt Manucredo wrote: > > Syzbot detects a shift-out-of-bounds in ___bpf_prog_run() > > kernel/bpf/core.c:1414:2. > > This is not enough. We need more information on why this happens > so we can judge whether the patch indeed fixed the issue. > > > > > I propose: In adjust_scalar_min_max_vals() move boundary check up to avoid > > missing them and return with error when detected. > > > > 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: > > ---------- > > 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 | 30 +++++++++--------------------- > > 1 file changed, 9 insertions(+), 21 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 94ba5163d4c5..ed0eecf20de5 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7510,6 +7510,15 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, > > u32_min_val = src_reg.u32_min_value; > > u32_max_val = src_reg.u32_max_value; > > > > + if ((opcode == BPF_LSH || opcode == BPF_RSH || opcode == BPF_ARSH) && > > + umax_val >= insn_bitness) { > > + /* Shifts greater than 31 or 63 are undefined. > > + * This includes shifts by a negative number. > > + */ > > + verbose(env, "invalid shift %lldn", umax_val); > > + return -EINVAL; > > + } > > I think your fix is good. I would like to move after > the following code though: > > if (!src_known && > opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND) { > __mark_reg_unknown(env, dst_reg); > return 0; > } > It can only be right before that code not after. That's the latest. In the case of the syzbot bug, opcode == BPF_LSH and !src_known. Therefore it needs to be before that block of code. > > + > > if (alu32) { > > src_known = tnum_subreg_is_const(src_reg.var_off); > > if ((src_known && > > @@ -7592,39 +7601,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; > > - } > > I think this is what happens. For the above case, we simply > marks the dst reg as unknown and didn't fail verification. > So later on at runtime, the shift optimization will have wrong > shift value (> 31/64). Please correct me if this is not right > analysis. As I mentioned in the early please write detailed > analysis in commit log. > Shouldn't the src reg be changed so that the shift-out-of-bounds can't occur, if return -EINVAL is not what we want here? Changing the dst reg might not help. If I look into kernel/bpf/core.c I can see: DST = DST OP SRC; > Please also add a test at tools/testing/selftests/bpf/verifier/. > I'm going to look into selftests, kind regards thanks, Kurt Manucredo _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2021-06-06 19:42 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-10 16:05 [syzbot] UBSAN: shift-out-of-bounds in ___bpf_prog_run syzbot 2021-03-28 3:38 ` syzbot 2021-06-02 21:27 ` [PATCH v3] bpf: core: fix " Kurt Manucredo 2021-06-02 21:27 ` Kurt Manucredo 2021-06-03 4:43 ` Greg KH 2021-06-03 4:43 ` Greg KH 2021-06-05 15:01 ` [PATCH v4] " Kurt Manucredo 2021-06-05 15:01 ` Kurt Manucredo 2021-06-05 17:55 ` Yonghong Song 2021-06-05 17:55 ` Yonghong Song via Linux-kernel-mentees 2021-06-05 19:10 ` Alexei Starovoitov 2021-06-05 19:10 ` Alexei Starovoitov 2021-06-05 21:39 ` Yonghong Song 2021-06-05 21:39 ` Yonghong Song via Linux-kernel-mentees 2021-06-06 19:44 ` Kurt Manucredo 2021-06-06 19:44 ` Kurt Manucredo 2021-06-07 7:38 ` Dmitry Vyukov 2021-06-07 7:38 ` Dmitry Vyukov 2021-06-07 7:38 ` Dmitry Vyukov via Linux-kernel-mentees 2021-06-09 18:20 ` Kees Cook 2021-06-09 18:20 ` Kees Cook 2021-06-09 23:40 ` Yonghong Song 2021-06-09 23:40 ` Yonghong Song via Linux-kernel-mentees 2021-06-10 5:32 ` Dmitry Vyukov 2021-06-10 5:32 ` Dmitry Vyukov 2021-06-10 5:32 ` Dmitry Vyukov via Linux-kernel-mentees 2021-06-10 6:06 ` Yonghong Song 2021-06-10 6:06 ` Yonghong Song via Linux-kernel-mentees 2021-06-10 17:06 ` Kees Cook 2021-06-10 17:06 ` Kees Cook 2021-06-10 17:52 ` Alexei Starovoitov 2021-06-10 17:52 ` Alexei Starovoitov 2021-06-10 17:52 ` Alexei Starovoitov 2021-06-10 20:00 ` Eric Biggers 2021-06-10 20:00 ` Eric Biggers 2021-06-15 16:42 ` [PATCH v5] " Kurt Manucredo 2021-06-15 18:51 ` 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 2021-06-06 19:15 ` Kurt Manucredo [this message] 2021-06-06 19:15 ` [PATCH v4] " Kurt Manucredo
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=20484-14561-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=gregkh@linuxfoundation.org \ --cc=hawk@kernel.org \ --cc=john.fastabend@gmail.com \ --cc=kafai@fb.com \ --cc=kpsingh@kernel.org \ --cc=kuba@kernel.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nathan@kernel.org \ --cc=ndesaulniers@google.com \ --cc=netdev@vger.kernel.org \ --cc=skhan@linuxfoundation.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.