* Re: UBSAN: array-index-out-of-bounds in arch_uprobe_analyze_insn [not found] <00000000000082559e05afc6b97a@google.com> @ 2020-09-22 4:20 ` syzbot 2020-12-02 0:48 ` Kees Cook 0 siblings, 1 reply; 3+ messages in thread From: syzbot @ 2020-09-22 4:20 UTC (permalink / raw) To: akpm, bp, coreteam, davem, gustavoars, hpa, john.stultz, kaber, kadlec, keescook, linux-kernel, mingo, netdev, netfilter-devel, pablo, syzkaller-bugs, tglx, torvalds, wang.yi59, x86 syzbot has bisected this issue to: commit 4b2bd5fec007a4fd3fc82474b9199af25013de4c Author: John Stultz <john.stultz@linaro.org> Date: Sat Oct 8 00:02:33 2016 +0000 proc: fix timerslack_ns CAP_SYS_NICE check when adjusting self bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1697348d900000 start commit: 325d0eab Merge branch 'akpm' (patches from Andrew) git tree: upstream final oops: https://syzkaller.appspot.com/x/report.txt?x=1597348d900000 console output: https://syzkaller.appspot.com/x/log.txt?x=1197348d900000 kernel config: https://syzkaller.appspot.com/x/.config?x=b12e84189082991c dashboard link: https://syzkaller.appspot.com/bug?extid=9b64b619f10f19d19a7c syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1573a8ad900000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=164ee6c5900000 Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com Fixes: 4b2bd5fec007 ("proc: fix timerslack_ns CAP_SYS_NICE check when adjusting self") For information about bisection process see: https://goo.gl/tpsmEJ#bisection ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in arch_uprobe_analyze_insn 2020-09-22 4:20 ` UBSAN: array-index-out-of-bounds in arch_uprobe_analyze_insn syzbot @ 2020-12-02 0:48 ` Kees Cook 2020-12-02 6:12 ` Masami Hiramatsu 0 siblings, 1 reply; 3+ messages in thread From: Kees Cook @ 2020-12-02 0:48 UTC (permalink / raw) To: Masami Hiramatsu, Srikar Dronamraju, Ricardo Neri Cc: akpm, bp, coreteam, syzbot, davem, gustavoars, hpa, john.stultz, kaber, kadlec, linux-kernel, mingo, netdev, netfilter-devel, pablo, syzkaller-bugs, tglx, torvalds, wang.yi59, x86 Hi, There appears to be a problem with prefix counting for the instruction decoder. It looks like insn_get_prefixes() isn't keeping "nb" and "nbytes" in sync correctly: while (inat_is_legacy_prefix(attr)) { /* Skip if same prefix */ for (i = 0; i < nb; i++) if (prefixes->bytes[i] == b) goto found; if (nb == 4) /* Invalid instruction */ break; prefixes->bytes[nb++] = b; ... found: prefixes->nbytes++; insn->next_byte++; lb = b; b = peek_next(insn_byte_t, insn); attr = inat_get_opcode_attribute(b); } (nbytes is incremented on repeated prefixes, but "nb" isn't) However, it looks like nbytes is used as an offset: static inline int insn_offset_rex_prefix(struct insn *insn) { return insn->prefixes.nbytes; } static inline int insn_offset_vex_prefix(struct insn *insn) { return insn_offset_rex_prefix(insn) + insn->rex_prefix.nbytes; } Which means everything that iterates over prefixes.bytes[] is buggy, since they may be trying to read past the end of the array: $ git grep -A3 -E '< .*prefixes(\.|->)nbytes' boot/compressed/sev-es.c: for (i = 0; i < insn->prefixes.nbytes; i++) { boot/compressed/sev-es.c- insn_byte_t p = insn->prefixes.bytes[i]; boot/compressed/sev-es.c- boot/compressed/sev-es.c- if (p == 0xf2 || p == 0xf3) -- kernel/uprobes.c: for (i = 0; i < insn->prefixes.nbytes; i++) { kernel/uprobes.c- insn_attr_t attr; kernel/uprobes.c- kernel/uprobes.c- attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]); -- kernel/uprobes.c: for (i = 0; i < insn->prefixes.nbytes; i++) { kernel/uprobes.c- if (insn->prefixes.bytes[i] == 0x66) kernel/uprobes.c- return -ENOTSUPP; kernel/uprobes.c- } -- lib/insn-eval.c: for (i = 0; i < insn->prefixes.nbytes; i++) { lib/insn-eval.c- insn_byte_t p = insn->prefixes.bytes[i]; lib/insn-eval.c- lib/insn-eval.c- if (p == 0xf2 || p == 0xf3) -- lib/insn-eval.c: for (i = 0; i < insn->prefixes.nbytes; i++) { lib/insn-eval.c- insn_attr_t attr; lib/insn-eval.c- lib/insn-eval.c- attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]); I don't see a clear way to fix this. -Kees On Mon, Sep 21, 2020 at 09:20:07PM -0700, syzbot wrote: > syzbot has bisected this issue to: > > commit 4b2bd5fec007a4fd3fc82474b9199af25013de4c > Author: John Stultz <john.stultz@linaro.org> > Date: Sat Oct 8 00:02:33 2016 +0000 > > proc: fix timerslack_ns CAP_SYS_NICE check when adjusting self > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1697348d900000 > start commit: 325d0eab Merge branch 'akpm' (patches from Andrew) > git tree: upstream > final oops: https://syzkaller.appspot.com/x/report.txt?x=1597348d900000 > console output: https://syzkaller.appspot.com/x/log.txt?x=1197348d900000 > kernel config: https://syzkaller.appspot.com/x/.config?x=b12e84189082991c > dashboard link: https://syzkaller.appspot.com/bug?extid=9b64b619f10f19d19a7c > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1573a8ad900000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=164ee6c5900000 > > Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com > Fixes: 4b2bd5fec007 ("proc: fix timerslack_ns CAP_SYS_NICE check when adjusting self") > > For information about bisection process see: https://goo.gl/tpsmEJ#bisection -- Kees Cook ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in arch_uprobe_analyze_insn 2020-12-02 0:48 ` Kees Cook @ 2020-12-02 6:12 ` Masami Hiramatsu 0 siblings, 0 replies; 3+ messages in thread From: Masami Hiramatsu @ 2020-12-02 6:12 UTC (permalink / raw) To: Kees Cook Cc: Srikar Dronamraju, Ricardo Neri, akpm, bp, coreteam, syzbot, davem, gustavoars, hpa, john.stultz, kaber, kadlec, linux-kernel, mingo, netdev, netfilter-devel, pablo, syzkaller-bugs, tglx, torvalds, wang.yi59, x86 Hi Kees, On Tue, 1 Dec 2020 16:48:55 -0800 Kees Cook <keescook@chromium.org> wrote: > Hi, > > There appears to be a problem with prefix counting for the instruction > decoder. It looks like insn_get_prefixes() isn't keeping "nb" and "nbytes" > in sync correctly: > > while (inat_is_legacy_prefix(attr)) { > /* Skip if same prefix */ > for (i = 0; i < nb; i++) > if (prefixes->bytes[i] == b) > goto found; > if (nb == 4) > /* Invalid instruction */ > break; > prefixes->bytes[nb++] = b; > ... > found: > prefixes->nbytes++; > insn->next_byte++; > lb = b; > b = peek_next(insn_byte_t, insn); > attr = inat_get_opcode_attribute(b); > } > > (nbytes is incremented on repeated prefixes, but "nb" isn't) > > However, it looks like nbytes is used as an offset: > > static inline int insn_offset_rex_prefix(struct insn *insn) > { > return insn->prefixes.nbytes; > } > static inline int insn_offset_vex_prefix(struct insn *insn) > { > return insn_offset_rex_prefix(insn) + insn->rex_prefix.nbytes; > } Yes, it is designed to do that. nbytes counts how many bytes the prefix is, and nb is how many bytes of the prefix->bytes consumed. Since the legacy prefix can be repeated and more than 4 (bytes), we can not store all of those. > > Which means everything that iterates over prefixes.bytes[] is buggy, > since they may be trying to read past the end of the array: Good catch! All following usage are wrong... > > $ git grep -A3 -E '< .*prefixes(\.|->)nbytes' > boot/compressed/sev-es.c: for (i = 0; i < insn->prefixes.nbytes; i++) { > boot/compressed/sev-es.c- insn_byte_t p = > insn->prefixes.bytes[i]; > boot/compressed/sev-es.c- > boot/compressed/sev-es.c- if (p == 0xf2 || p == 0xf3) > -- > kernel/uprobes.c: for (i = 0; i < insn->prefixes.nbytes; i++) { > kernel/uprobes.c- insn_attr_t attr; > kernel/uprobes.c- > kernel/uprobes.c- attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]); > -- > kernel/uprobes.c: for (i = 0; i < insn->prefixes.nbytes; i++) { > kernel/uprobes.c- if (insn->prefixes.bytes[i] == 0x66) > kernel/uprobes.c- return -ENOTSUPP; > kernel/uprobes.c- } > -- > lib/insn-eval.c: for (i = 0; i < insn->prefixes.nbytes; i++) { > lib/insn-eval.c- insn_byte_t p = insn->prefixes.bytes[i]; > lib/insn-eval.c- > lib/insn-eval.c- if (p == 0xf2 || p == 0xf3) > -- > lib/insn-eval.c: for (i = 0; i < insn->prefixes.nbytes; i++) { > lib/insn-eval.c- insn_attr_t attr; > lib/insn-eval.c- > lib/insn-eval.c- attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]); > > I don't see a clear way to fix this. For the loop, we can check the insn.prefixes.bytes[i] == 0 since it is initialized by 0 and 0x0 is not a prefix like this. for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) { ... } Thank you, > > -Kees > > On Mon, Sep 21, 2020 at 09:20:07PM -0700, syzbot wrote: > > syzbot has bisected this issue to: > > > > commit 4b2bd5fec007a4fd3fc82474b9199af25013de4c > > Author: John Stultz <john.stultz@linaro.org> > > Date: Sat Oct 8 00:02:33 2016 +0000 > > > > proc: fix timerslack_ns CAP_SYS_NICE check when adjusting self > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1697348d900000 > > start commit: 325d0eab Merge branch 'akpm' (patches from Andrew) > > git tree: upstream > > final oops: https://syzkaller.appspot.com/x/report.txt?x=1597348d900000 > > console output: https://syzkaller.appspot.com/x/log.txt?x=1197348d900000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=b12e84189082991c > > dashboard link: https://syzkaller.appspot.com/bug?extid=9b64b619f10f19d19a7c > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1573a8ad900000 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=164ee6c5900000 > > > > Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com > > Fixes: 4b2bd5fec007 ("proc: fix timerslack_ns CAP_SYS_NICE check when adjusting self") > > > > For information about bisection process see: https://goo.gl/tpsmEJ#bisection > > -- > Kees Cook -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-02 6:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <00000000000082559e05afc6b97a@google.com>
2020-09-22 4:20 ` UBSAN: array-index-out-of-bounds in arch_uprobe_analyze_insn syzbot
2020-12-02 0:48 ` Kees Cook
2020-12-02 6:12 ` Masami Hiramatsu
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).