| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y4jDhHDQD3wY6g8C@FVFF77S0Q05N>
Date: Thu, 1 Dec 2022 15:08:52 +0000
From: Mark Rutland <mark.rutland@....com>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Mark Brown <broonie@...nel.org>,
Kalesh Singh <kaleshsingh@...gle.com>,
Marc Zyngier <maz@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
Subject: Re: [PATCH 3/3] arm64: kprobes: Return DBG_HOOK_ERROR if kprobes can
not handle a BRK
On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@...nel.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.
IIUC this relies on BRK handling not being preemptible, which is something
we've repeatedly considered changing along with a bunch of other debug
exception handling.
In case we do try to change that in future, it would be good to have a comment
somewhere to that effect.
I think there are other ways we could synchronise against that (e.g. using RCU
tasks rude) if we ever do that, and this patch looks good to me.
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> ---
> arch/arm64/kernel/probes/kprobes.c | 79 +++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d2ae37f89774..ea56b22d4da8 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,45 @@ 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 must be put by kprobe, but we
> + * could not find corresponding kprobes. Let the kernel handle
> + * this error case.
> + */
Could we make this:
/*
* 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 no need to single
> + * stepping. Let's just reset current kprobe and exit.
> + */
Minor wording nit: could we replace:
no need to single stepping.
With:
not need to single-step.
Thanks,
Mark.
> + 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 +372,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).
>
Powered by blists - more mailing lists