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
| ||
|
Date: Fri, 2 Dec 2022 09:42:58 +0900 From: Masami Hiramatsu (Google) <mhiramat@...nel.org> To: Mark Rutland <mark.rutland@....com> 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, 1 Dec 2022 17:21:55 +0000 Mark Rutland <mark.rutland@....com> wrote: > On Fri, Dec 02, 2022 at 01:07:13AM +0900, Masami Hiramatsu wrote: > > On Thu, 1 Dec 2022 15:08:52 +0000 > > Mark Rutland <mark.rutland@....com> wrote: > > > > > 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. > > > > Interesting idea... and it also need many changes in kprobe itself. > > > > > > > > In case we do try to change that in future, it would be good to have a comment > > > somewhere to that effect. > > > > Hmm, it would fundamentally change the assumptions that kprobes relies on, > > and would require a lot of thought again. (e.g. current running kprobe is > > stored in per-cpu variable, it should be per-task. etc.) > > Ah; I had not considered that. > > Feel free to ignore the above; with the comments as below: > > Acked-by: Mark Rutland <mark.rutland@....com> OK, Thanks! > > Thanks, > Mark. > > > > > > > > > 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. > > > */ > > > > OK. > > > > > > > > > + 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. > > > > OK, I'll update both in v2. > > > > Thank you! > > > > > > > > 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). > > > > > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@...nel.org> -- Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists