From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org> To: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, Mark Brown <broonie@kernel.org>, Kalesh Singh <kaleshsingh@google.com>, Masami Hiramatsu <mhiramat@kernel.org>, Marc Zyngier <maz@kernel.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com> Subject: [PATCH v2 3/3] arm64: kprobes: Return DBG_HOOK_ERROR if kprobes can not handle a BRK Date: Fri, 2 Dec 2022 11:18:52 +0900 [thread overview] Message-ID: <166994753273.439920.6629626290560350760.stgit@devnote3> (raw) In-Reply-To: <166994750386.439920.1754385804350980158.stgit@devnote3> From: Masami Hiramatsu (Google) <mhiramat@kernel.org> Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it fails to find a kprobe corresponding to the address. Since arm64 kprobes uses stop_machine based text patching for removing BRK, it ensures all running kprobe_break_handler() is done at that point. And after removing the BRK, it removes the kprobe from its hash list. Thus, if the kprobe_break_handler() fails to find kprobe from hash list, there is a bug. Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Acked-by: Mark Rutland <mark.rutland@arm.com> --- Changes in v2: - Update comments in the code. --- arch/arm64/kernel/probes/kprobes.c | 78 +++++++++++++++++------------------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d2ae37f89774..f35d059a9a36 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr) return 0; } -static void __kprobes kprobe_handler(struct pt_regs *regs) +static int __kprobes +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) { struct kprobe *p, *cur_kprobe; struct kprobe_ctlblk *kcb; @@ -308,39 +309,44 @@ static void __kprobes kprobe_handler(struct pt_regs *regs) cur_kprobe = kprobe_running(); p = get_kprobe((kprobe_opcode_t *) addr); + if (WARN_ON_ONCE(!p)) { + /* + * Something went wrong. This BRK used an immediate reserved + * for kprobes, but we couldn't find any corresponding probe. + */ + return DBG_HOOK_ERROR; + } - if (p) { - if (cur_kprobe) { - if (reenter_kprobe(p, regs, kcb)) - return; - } else { - /* Probe hit */ - set_current_kprobe(p); - kcb->kprobe_status = KPROBE_HIT_ACTIVE; - - /* - * If we have no pre-handler or it returned 0, we - * continue with normal processing. If we have a - * pre-handler and it returned non-zero, it will - * modify the execution path and no need to single - * stepping. Let's just reset current kprobe and exit. - */ - if (!p->pre_handler || !p->pre_handler(p, regs)) { - setup_singlestep(p, regs, kcb, 0); - } else - reset_current_kprobe(); - } + if (cur_kprobe) { + /* Hit a kprobe inside another kprobe */ + if (!reenter_kprobe(p, regs, kcb)) + return DBG_HOOK_ERROR; + } else { + /* Probe hit */ + set_current_kprobe(p); + kcb->kprobe_status = KPROBE_HIT_ACTIVE; + + /* + * If we have no pre-handler or it returned 0, we + * continue with normal processing. If we have a + * pre-handler and it returned non-zero, it will + * modify the execution path and not need to single-step + * Let's just reset current kprobe and exit. + */ + if (!p->pre_handler || !p->pre_handler(p, regs)) + setup_singlestep(p, regs, kcb, 0); + else + reset_current_kprobe(); } - /* - * The breakpoint instruction was removed right - * after we hit it. Another cpu has removed - * either a probepoint or a debugger breakpoint - * at this address. In either case, no further - * handling of this interrupt is appropriate. - * Return back to original instruction, and continue. - */ + + return DBG_HOOK_HANDLED; } +static struct break_hook kprobes_break_hook = { + .imm = KPROBES_BRK_IMM, + .fn = kprobe_breakpoint_handler, +}; + static int __kprobes kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr) { @@ -365,18 +371,6 @@ static struct break_hook kprobes_break_ss_hook = { .fn = kprobe_breakpoint_ss_handler, }; -static int __kprobes -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) -{ - kprobe_handler(regs); - return DBG_HOOK_HANDLED; -} - -static struct break_hook kprobes_break_hook = { - .imm = KPROBES_BRK_IMM, - .fn = kprobe_breakpoint_handler, -}; - /* * Provide a blacklist of symbols identifying ranges which cannot be kprobed. * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
WARNING: multiple messages have this Message-ID (diff)
From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org> To: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, Mark Brown <broonie@kernel.org>, Kalesh Singh <kaleshsingh@google.com>, Masami Hiramatsu <mhiramat@kernel.org>, Marc Zyngier <maz@kernel.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com> Subject: [PATCH v2 3/3] arm64: kprobes: Return DBG_HOOK_ERROR if kprobes can not handle a BRK Date: Fri, 2 Dec 2022 11:18:52 +0900 [thread overview] Message-ID: <166994753273.439920.6629626290560350760.stgit@devnote3> (raw) In-Reply-To: <166994750386.439920.1754385804350980158.stgit@devnote3> From: Masami Hiramatsu (Google) <mhiramat@kernel.org> Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it fails to find a kprobe corresponding to the address. Since arm64 kprobes uses stop_machine based text patching for removing BRK, it ensures all running kprobe_break_handler() is done at that point. And after removing the BRK, it removes the kprobe from its hash list. Thus, if the kprobe_break_handler() fails to find kprobe from hash list, there is a bug. Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Acked-by: Mark Rutland <mark.rutland@arm.com> --- Changes in v2: - Update comments in the code. --- arch/arm64/kernel/probes/kprobes.c | 78 +++++++++++++++++------------------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d2ae37f89774..f35d059a9a36 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr) return 0; } -static void __kprobes kprobe_handler(struct pt_regs *regs) +static int __kprobes +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) { struct kprobe *p, *cur_kprobe; struct kprobe_ctlblk *kcb; @@ -308,39 +309,44 @@ static void __kprobes kprobe_handler(struct pt_regs *regs) cur_kprobe = kprobe_running(); p = get_kprobe((kprobe_opcode_t *) addr); + if (WARN_ON_ONCE(!p)) { + /* + * Something went wrong. This BRK used an immediate reserved + * for kprobes, but we couldn't find any corresponding probe. + */ + return DBG_HOOK_ERROR; + } - if (p) { - if (cur_kprobe) { - if (reenter_kprobe(p, regs, kcb)) - return; - } else { - /* Probe hit */ - set_current_kprobe(p); - kcb->kprobe_status = KPROBE_HIT_ACTIVE; - - /* - * If we have no pre-handler or it returned 0, we - * continue with normal processing. If we have a - * pre-handler and it returned non-zero, it will - * modify the execution path and no need to single - * stepping. Let's just reset current kprobe and exit. - */ - if (!p->pre_handler || !p->pre_handler(p, regs)) { - setup_singlestep(p, regs, kcb, 0); - } else - reset_current_kprobe(); - } + if (cur_kprobe) { + /* Hit a kprobe inside another kprobe */ + if (!reenter_kprobe(p, regs, kcb)) + return DBG_HOOK_ERROR; + } else { + /* Probe hit */ + set_current_kprobe(p); + kcb->kprobe_status = KPROBE_HIT_ACTIVE; + + /* + * If we have no pre-handler or it returned 0, we + * continue with normal processing. If we have a + * pre-handler and it returned non-zero, it will + * modify the execution path and not need to single-step + * Let's just reset current kprobe and exit. + */ + if (!p->pre_handler || !p->pre_handler(p, regs)) + setup_singlestep(p, regs, kcb, 0); + else + reset_current_kprobe(); } - /* - * The breakpoint instruction was removed right - * after we hit it. Another cpu has removed - * either a probepoint or a debugger breakpoint - * at this address. In either case, no further - * handling of this interrupt is appropriate. - * Return back to original instruction, and continue. - */ + + return DBG_HOOK_HANDLED; } +static struct break_hook kprobes_break_hook = { + .imm = KPROBES_BRK_IMM, + .fn = kprobe_breakpoint_handler, +}; + static int __kprobes kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr) { @@ -365,18 +371,6 @@ static struct break_hook kprobes_break_ss_hook = { .fn = kprobe_breakpoint_ss_handler, }; -static int __kprobes -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) -{ - kprobe_handler(regs); - return DBG_HOOK_HANDLED; -} - -static struct break_hook kprobes_break_hook = { - .imm = KPROBES_BRK_IMM, - .fn = kprobe_breakpoint_handler, -}; - /* * Provide a blacklist of symbols identifying ranges which cannot be kprobed. * This blacklist is exposed to userspace via debugfs (kprobes/blacklist). _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-12-02 2:19 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-02 2:18 [PATCH v2 0/3] arm64: kprobes: Fix bugs in kprobes for arm64 Masami Hiramatsu (Google) 2022-12-02 2:18 ` Masami Hiramatsu (Google) 2022-12-02 2:18 ` [PATCH v2 1/3] arm64: Prohibit instrumentation on arch_stack_walk() Masami Hiramatsu (Google) 2022-12-02 2:18 ` Masami Hiramatsu (Google) 2022-12-02 13:17 ` Mark Rutland 2022-12-02 13:17 ` Mark Rutland 2022-12-06 0:41 ` Masami Hiramatsu 2022-12-06 0:41 ` Masami Hiramatsu 2022-12-09 11:16 ` Mark Rutland 2022-12-09 11:16 ` Mark Rutland 2022-12-11 3:30 ` Masami Hiramatsu 2022-12-11 3:30 ` Masami Hiramatsu 2022-12-02 2:18 ` [PATCH v2 2/3] arm64: kprobes: Let arch do_page_fault() fix up page fault in user handler Masami Hiramatsu (Google) 2022-12-02 2:18 ` Masami Hiramatsu (Google) 2022-12-02 2:18 ` Masami Hiramatsu (Google) [this message] 2022-12-02 2:18 ` [PATCH v2 3/3] arm64: kprobes: Return DBG_HOOK_ERROR if kprobes can not handle a BRK Masami Hiramatsu (Google) 2022-12-05 15:43 ` [PATCH v2 0/3] arm64: kprobes: Fix bugs in kprobes for arm64 Will Deacon 2022-12-05 15:43 ` Will Deacon
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=166994753273.439920.6629626290560350760.stgit@devnote3 \ --to=mhiramat@kernel.org \ --cc=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=kaleshsingh@google.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=sandeepa.s.prabhu@gmail.com \ --cc=will@kernel.org \ /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.