| Message ID | 166990556124.253128.2968612748605960211.stgit@devnote3 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: kprobes: Fix bugs in kprobes for arm64 | expand |
On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote: > 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. 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@kernel.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). >
On Thu, 1 Dec 2022 15:08:52 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote: > > 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. > > 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.) > > 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@kernel.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). > >
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@arm.com> wrote: > > > On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote: > > > 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. > > > > 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@arm.com> 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@kernel.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@kernel.org>
On Thu, 1 Dec 2022 17:21:55 +0000 Mark Rutland <mark.rutland@arm.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@arm.com> wrote: > > > > > On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote: > > > > 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. > > > > > > 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@arm.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@kernel.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@kernel.org>
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. + */ + 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. + */ + 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).