* [PATCH 0/2] lkdtm: fix irq handler entry for arm64 @ 2018-02-01 9:34 AKASHI Takahiro 2018-02-01 9:34 ` [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check AKASHI Takahiro 2018-02-01 9:34 ` [PATCH 2/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro 0 siblings, 2 replies; 13+ messages in thread From: AKASHI Takahiro @ 2018-02-01 9:34 UTC (permalink / raw) To: linux-arm-kernel Lkdtm is a handy way for testing kdump in case of panic in interrupt context: echo PANIC > /sys/kernel/debug/provoke-crash/INT_HARDWARE_ENTRY But this just doesn't work on arm64 since "do_IRQ" function doesn't exist on arm64. This patch series addresses this problem. Patch#1 is a preliminary fix for patch#2 and allows for probing any exception entries, including irq handler. For further improvements, some of other hooks doesn't work: * handle_IRQ_event() was removed in v2.6.39 * hrtimer_start() was converted to an inline function in v4.2 but I don't know how we should deal with them. AKASHI Takahiro (1): lkdtm: fix irq handler entry for arm64 Masami Hiramatsu (1): arm64: kprobes: Remove unneeded address sanity check arch/arm64/kernel/probes/kprobes.c | 8 -------- drivers/misc/lkdtm_core.c | 20 ++++++++++++++++++-- 2 files changed, 18 insertions(+), 10 deletions(-) -- 2.15.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-01 9:34 [PATCH 0/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro @ 2018-02-01 9:34 ` AKASHI Takahiro 2018-02-06 14:36 ` Will Deacon 2018-02-15 2:08 ` David Long 2018-02-01 9:34 ` [PATCH 2/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro 1 sibling, 2 replies; 13+ messages in thread From: AKASHI Takahiro @ 2018-02-01 9:34 UTC (permalink / raw) To: linux-arm-kernel From: Masami Hiramatsu <mhiramat@kernel.org> Remove unneeded address sanity check in arch_prepare_kprobe(). Since do_debug_exception() is already blacklisted for kprobes, no need to reject all __exception functions. Also, since generic kprobe framework already ensures the address is in kernel text, no need to check it is in rodata again. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm64/kernel/probes/kprobes.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d849d9804011..3c487a389252 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) int __kprobes arch_prepare_kprobe(struct kprobe *p) { unsigned long probe_addr = (unsigned long)p->addr; - extern char __start_rodata[]; - extern char __end_rodata[]; if (probe_addr & 0x3) return -EINVAL; @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) /* copy instruction */ p->opcode = le32_to_cpu(*p->addr); - if (in_exception_text(probe_addr)) - return -EINVAL; - if (probe_addr >= (unsigned long) __start_rodata && - probe_addr <= (unsigned long) __end_rodata) - return -EINVAL; - /* decode instruction */ switch (arm_kprobe_decode_insn(p->addr, &p->ainsn)) { case INSN_REJECTED: /* insn not supported */ -- 2.15.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-01 9:34 ` [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check AKASHI Takahiro @ 2018-02-06 14:36 ` Will Deacon 2018-02-07 0:02 ` Masami Hiramatsu 2018-02-15 2:08 ` David Long 1 sibling, 1 reply; 13+ messages in thread From: Will Deacon @ 2018-02-06 14:36 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 01, 2018 at 06:34:58PM +0900, AKASHI Takahiro wrote: > From: Masami Hiramatsu <mhiramat@kernel.org> > > Remove unneeded address sanity check in arch_prepare_kprobe(). > Since do_debug_exception() is already blacklisted for kprobes, no need > to reject all __exception functions. Also, since generic kprobe > framework already ensures the address is in kernel text, no need to > check it is in rodata again. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm64/kernel/probes/kprobes.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > index d849d9804011..3c487a389252 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c > @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) > int __kprobes arch_prepare_kprobe(struct kprobe *p) > { > unsigned long probe_addr = (unsigned long)p->addr; > - extern char __start_rodata[]; > - extern char __end_rodata[]; > > if (probe_addr & 0x3) > return -EINVAL; > @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) > /* copy instruction */ > p->opcode = le32_to_cpu(*p->addr); > > - if (in_exception_text(probe_addr)) > - return -EINVAL; > - if (probe_addr >= (unsigned long) __start_rodata && > - probe_addr <= (unsigned long) __end_rodata) > - return -EINVAL; > - Does this mean we can remove in_exception_text and the corresponding __exception annotations from arm64? Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-06 14:36 ` Will Deacon @ 2018-02-07 0:02 ` Masami Hiramatsu 0 siblings, 0 replies; 13+ messages in thread From: Masami Hiramatsu @ 2018-02-07 0:02 UTC (permalink / raw) To: linux-arm-kernel Hi Will, 2018-02-06 23:36 GMT+09:00 Will Deacon <will.deacon@arm.com>: > On Thu, Feb 01, 2018 at 06:34:58PM +0900, AKASHI Takahiro wrote: >> From: Masami Hiramatsu <mhiramat@kernel.org> >> >> Remove unneeded address sanity check in arch_prepare_kprobe(). >> Since do_debug_exception() is already blacklisted for kprobes, no need >> to reject all __exception functions. Also, since generic kprobe >> framework already ensures the address is in kernel text, no need to >> check it is in rodata again. >> >> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> >> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> arch/arm64/kernel/probes/kprobes.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c >> index d849d9804011..3c487a389252 100644 >> --- a/arch/arm64/kernel/probes/kprobes.c >> +++ b/arch/arm64/kernel/probes/kprobes.c >> @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) >> int __kprobes arch_prepare_kprobe(struct kprobe *p) >> { >> unsigned long probe_addr = (unsigned long)p->addr; >> - extern char __start_rodata[]; >> - extern char __end_rodata[]; >> >> if (probe_addr & 0x3) >> return -EINVAL; >> @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) >> /* copy instruction */ >> p->opcode = le32_to_cpu(*p->addr); >> >> - if (in_exception_text(probe_addr)) >> - return -EINVAL; >> - if (probe_addr >= (unsigned long) __start_rodata && >> - probe_addr <= (unsigned long) __end_rodata) >> - return -EINVAL; >> - > > Does this mean we can remove in_exception_text and the corresponding > __exception annotations from arm64? Good catch. As far as I can see, there is no other in_exception_text() user under arch/arm64, so we can remove it all. Thank you, -- Masami Hiramatsu ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-01 9:34 ` [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check AKASHI Takahiro 2018-02-06 14:36 ` Will Deacon @ 2018-02-15 2:08 ` David Long 2018-02-15 6:47 ` Masami Hiramatsu 1 sibling, 1 reply; 13+ messages in thread From: David Long @ 2018-02-15 2:08 UTC (permalink / raw) To: linux-arm-kernel On 02/01/2018 04:34 AM, AKASHI Takahiro wrote: > From: Masami Hiramatsu <mhiramat@kernel.org> > > Remove unneeded address sanity check in arch_prepare_kprobe(). > Since do_debug_exception() is already blacklisted for kprobes, no need > to reject all __exception functions. Also, since generic kprobe > framework already ensures the address is in kernel text, no need to > check it is in rodata again. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm64/kernel/probes/kprobes.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > index d849d9804011..3c487a389252 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c > @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) > int __kprobes arch_prepare_kprobe(struct kprobe *p) > { > unsigned long probe_addr = (unsigned long)p->addr; > - extern char __start_rodata[]; > - extern char __end_rodata[]; > > if (probe_addr & 0x3) > return -EINVAL; > @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) > /* copy instruction */ > p->opcode = le32_to_cpu(*p->addr); > > - if (in_exception_text(probe_addr)) > - return -EINVAL; > - if (probe_addr >= (unsigned long) __start_rodata && > - probe_addr <= (unsigned long) __end_rodata) > - return -EINVAL; > - > /* decode instruction */ > switch (arm_kprobe_decode_insn(p->addr, &p->ainsn)) { > case INSN_REJECTED: /* insn not supported */ > I have tested this change on v4.15 using kprobes events and I find it allows kprobes to be placed in exception text when they were previously rejected. Is there some other recent change I need to test this with for the previous behavior to be preserved? -dl ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-15 2:08 ` David Long @ 2018-02-15 6:47 ` Masami Hiramatsu 2018-02-22 5:19 ` David Long 0 siblings, 1 reply; 13+ messages in thread From: Masami Hiramatsu @ 2018-02-15 6:47 UTC (permalink / raw) To: linux-arm-kernel Hi David, On Wed, 14 Feb 2018 21:08:03 -0500 David Long <dave.long@linaro.org> wrote: > On 02/01/2018 04:34 AM, AKASHI Takahiro wrote: > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > Remove unneeded address sanity check in arch_prepare_kprobe(). > > Since do_debug_exception() is already blacklisted for kprobes, no need > > to reject all __exception functions. Also, since generic kprobe > > framework already ensures the address is in kernel text, no need to > > check it is in rodata again. > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > arch/arm64/kernel/probes/kprobes.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > > index d849d9804011..3c487a389252 100644 > > --- a/arch/arm64/kernel/probes/kprobes.c > > +++ b/arch/arm64/kernel/probes/kprobes.c > > @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) > > int __kprobes arch_prepare_kprobe(struct kprobe *p) > > { > > unsigned long probe_addr = (unsigned long)p->addr; > > - extern char __start_rodata[]; > > - extern char __end_rodata[]; > > > > if (probe_addr & 0x3) > > return -EINVAL; > > @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) > > /* copy instruction */ > > p->opcode = le32_to_cpu(*p->addr); > > > > - if (in_exception_text(probe_addr)) > > - return -EINVAL; > > - if (probe_addr >= (unsigned long) __start_rodata && > > - probe_addr <= (unsigned long) __end_rodata) > > - return -EINVAL; > > - > > /* decode instruction */ > > switch (arm_kprobe_decode_insn(p->addr, &p->ainsn)) { > > case INSN_REJECTED: /* insn not supported */ > > > > I have tested this change on v4.15 using kprobes events and I find it > allows kprobes to be placed in exception text when they were previously > rejected. Is there some other recent change I need to test this with for > the previous behavior to be preserved? Hmm, the latest change is to avoid retpoline thunk functions on x86. Since the retpoline may not be applied on aarch64, it can be ignored. However, I found there were still many "__kprobes" tags under arch/arm64. That was replaced with NOKPROBE_SYMBOL() (and nokprobe_inline for inline function). It should be done on arm/arm64 too because the functions marked with NOKPROBE_SYMBOL are listed in <debugfs>/kprobes/blacklist. Thank you, > > -dl > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-15 6:47 ` Masami Hiramatsu @ 2018-02-22 5:19 ` David Long 2018-02-22 5:45 ` Masami Hiramatsu 0 siblings, 1 reply; 13+ messages in thread From: David Long @ 2018-02-22 5:19 UTC (permalink / raw) To: linux-arm-kernel On 02/15/2018 01:47 AM, Masami Hiramatsu wrote: > Hi David, > > On Wed, 14 Feb 2018 21:08:03 -0500 > David Long <dave.long@linaro.org> wrote: > >> On 02/01/2018 04:34 AM, AKASHI Takahiro wrote: >>> From: Masami Hiramatsu <mhiramat@kernel.org> >>> >>> Remove unneeded address sanity check in arch_prepare_kprobe(). >>> Since do_debug_exception() is already blacklisted for kprobes, no need >>> to reject all __exception functions. Also, since generic kprobe >>> framework already ensures the address is in kernel text, no need to >>> check it is in rodata again. >>> >>> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> >>> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> arch/arm64/kernel/probes/kprobes.c | 8 -------- >>> 1 file changed, 8 deletions(-) >>> >>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c >>> index d849d9804011..3c487a389252 100644 >>> --- a/arch/arm64/kernel/probes/kprobes.c >>> +++ b/arch/arm64/kernel/probes/kprobes.c >>> @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) >>> int __kprobes arch_prepare_kprobe(struct kprobe *p) >>> { >>> unsigned long probe_addr = (unsigned long)p->addr; >>> - extern char __start_rodata[]; >>> - extern char __end_rodata[]; >>> >>> if (probe_addr & 0x3) >>> return -EINVAL; >>> @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) >>> /* copy instruction */ >>> p->opcode = le32_to_cpu(*p->addr); >>> >>> - if (in_exception_text(probe_addr)) >>> - return -EINVAL; >>> - if (probe_addr >= (unsigned long) __start_rodata && >>> - probe_addr <= (unsigned long) __end_rodata) >>> - return -EINVAL; >>> - >>> /* decode instruction */ >>> switch (arm_kprobe_decode_insn(p->addr, &p->ainsn)) { >>> case INSN_REJECTED: /* insn not supported */ >>> >> >> I have tested this change on v4.15 using kprobes events and I find it >> allows kprobes to be placed in exception text when they were previously >> rejected. Is there some other recent change I need to test this with for >> the previous behavior to be preserved? > > Hmm, the latest change is to avoid retpoline thunk functions on x86. Since the > retpoline may not be applied on aarch64, it can be ignored. > However, I found there were still many "__kprobes" tags under arch/arm64. That > was replaced with NOKPROBE_SYMBOL() (and nokprobe_inline for inline function). > It should be done on arm/arm64 too because the functions marked with > NOKPROBE_SYMBOL are listed in <debugfs>/kprobes/blacklist. My bad for not reading the whole patch set before commenting. I understand the goal now. I see NOKPROBE_SYMBOL is only used for a few architectures so far, with arm64 widely using both methods. I'm presuming this is work in progress. I verified do_debug_exception is still rejected by kprobes. The other global functions in there are accepted after the change. Do we think that's safe? I can't immediately come up with a reason it wouldn't be. Has it been tested, beyond the IRQ stuff? I remember adding the rodata test. Seems to me there was a reason for that at the time, but I've verified that probes in rodata are still rejected after the patch. -dl ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check 2018-02-22 5:19 ` David Long @ 2018-02-22 5:45 ` Masami Hiramatsu 0 siblings, 0 replies; 13+ messages in thread From: Masami Hiramatsu @ 2018-02-22 5:45 UTC (permalink / raw) To: linux-arm-kernel Hi David, 2018-02-22 14:19 GMT+09:00 David Long <dave.long@linaro.org>: > On 02/15/2018 01:47 AM, Masami Hiramatsu wrote: >> >> Hi David, >> >> On Wed, 14 Feb 2018 21:08:03 -0500 >> David Long <dave.long@linaro.org> wrote: >> >>> On 02/01/2018 04:34 AM, AKASHI Takahiro wrote: >>>> >>>> From: Masami Hiramatsu <mhiramat@kernel.org> >>>> >>>> Remove unneeded address sanity check in arch_prepare_kprobe(). >>>> Since do_debug_exception() is already blacklisted for kprobes, no need >>>> to reject all __exception functions. Also, since generic kprobe >>>> framework already ensures the address is in kernel text, no need to >>>> check it is in rodata again. >>>> >>>> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> >>>> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> --- >>>> arch/arm64/kernel/probes/kprobes.c | 8 -------- >>>> 1 file changed, 8 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/probes/kprobes.c >>>> b/arch/arm64/kernel/probes/kprobes.c >>>> index d849d9804011..3c487a389252 100644 >>>> --- a/arch/arm64/kernel/probes/kprobes.c >>>> +++ b/arch/arm64/kernel/probes/kprobes.c >>>> @@ -78,8 +78,6 @@ static void __kprobes arch_simulate_insn(struct kprobe >>>> *p, struct pt_regs *regs) >>>> int __kprobes arch_prepare_kprobe(struct kprobe *p) >>>> { >>>> unsigned long probe_addr = (unsigned long)p->addr; >>>> - extern char __start_rodata[]; >>>> - extern char __end_rodata[]; >>>> >>>> if (probe_addr & 0x3) >>>> return -EINVAL; >>>> @@ -87,12 +85,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) >>>> /* copy instruction */ >>>> p->opcode = le32_to_cpu(*p->addr); >>>> >>>> - if (in_exception_text(probe_addr)) >>>> - return -EINVAL; >>>> - if (probe_addr >= (unsigned long) __start_rodata && >>>> - probe_addr <= (unsigned long) __end_rodata) >>>> - return -EINVAL; >>>> - >>>> /* decode instruction */ >>>> switch (arm_kprobe_decode_insn(p->addr, &p->ainsn)) { >>>> case INSN_REJECTED: /* insn not supported */ >>>> >>> >>> I have tested this change on v4.15 using kprobes events and I find it >>> allows kprobes to be placed in exception text when they were previously >>> rejected. Is there some other recent change I need to test this with for >>> the previous behavior to be preserved? >> >> >> Hmm, the latest change is to avoid retpoline thunk functions on x86. Since >> the >> retpoline may not be applied on aarch64, it can be ignored. >> However, I found there were still many "__kprobes" tags under arch/arm64. >> That >> was replaced with NOKPROBE_SYMBOL() (and nokprobe_inline for inline >> function). >> It should be done on arm/arm64 too because the functions marked with >> NOKPROBE_SYMBOL are listed in <debugfs>/kprobes/blacklist. > > > My bad for not reading the whole patch set before commenting. I understand > the goal now. > > I see NOKPROBE_SYMBOL is only used for a few architectures so far, with > arm64 widely using both methods. I'm presuming this is work in progress. > > I verified do_debug_exception is still rejected by kprobes. The other global > functions in there are accepted after the change. Do we think that's safe? I > can't immediately come up with a reason it wouldn't be. Has it been tested, > beyond the IRQ stuff? Yeah, it is reasonable concern. We already have ftrace/kprobe interface in debugfs (tracefs) so we can start testing it now with, something like below shell script; n=0 cd /sys/kernel/debug/tracing cut -f 3 -d " " /proc/kallsyms | while read sym; do echo "probing $sym" echo "p $sym" >> kprobe_events n=$((n+1)) [ $n -gt 3000 ] && break done I think it could be enough to test first 2-3k syms since most of arch dependent code are placed there. See my old slide. https://events.static.linuxfound.org/sites/events/files/slides/Handling%20the%20Massive%20Multiple%20Kprobes%20v2_1.pdf Unfortunately, kpcache and hash-table expansion (most critical features) were not accepted, so take care of performance degradation. > I remember adding the rodata test. Seems to me there was a reason for that > at the time, but I've verified that probes in rodata are still rejected > after the patch. Yes, because kernel/kprobes.c accepts the probes probing the kernel .text. Thank you, -- Masami Hiramatsu ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] lkdtm: fix irq handler entry for arm64 2018-02-01 9:34 [PATCH 0/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro 2018-02-01 9:34 ` [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check AKASHI Takahiro @ 2018-02-01 9:34 ` AKASHI Takahiro 2018-02-27 3:57 ` Kees Cook 1 sibling, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2018-02-01 9:34 UTC (permalink / raw) To: linux-arm-kernel Arm64 doesn't have "do_IRQ" function, instead *handle_arch_irq, which is initialized by irq chip (gic), is called from exception entry. This patch fixes this problem. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- drivers/misc/lkdtm_core.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c index ba92291508dc..e20343543053 100644 --- a/drivers/misc/lkdtm_core.c +++ b/drivers/misc/lkdtm_core.c @@ -249,13 +249,29 @@ static int lkdtm_register_cpoint(struct crashpoint *crashpoint, if (lkdtm_kprobe != NULL) unregister_kprobe(lkdtm_kprobe); + if (IS_ENABLED(CONFIG_ARM64) && + !strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { + extern void (*handle_arch_irq)(struct pt_regs *regs); + + crashpoint->kprobe.addr = (kprobe_opcode_t *)*handle_arch_irq; + /* + * Instantiating kprobe.symbol_name here, say + * with lookup_symbol_name(*handle_arch_irq), + * would cause register_kprobe() to fail. + */ + crashpoint->kprobe.symbol_name = NULL; + } lkdtm_crashpoint = crashpoint; lkdtm_crashtype = crashtype; lkdtm_kprobe = &crashpoint->kprobe; ret = register_kprobe(lkdtm_kprobe); if (ret < 0) { - pr_info("Couldn't register kprobe %s\n", - crashpoint->kprobe.symbol_name); + if (IS_ENABLED(CONFIG_ARM64)) + pr_info("Couldn't register kprobe 0x%lx\n", + (unsigned long)crashpoint->kprobe.addr); + else + pr_info("Couldn't register kprobe %s\n", + crashpoint->kprobe.symbol_name); lkdtm_kprobe = NULL; lkdtm_crashpoint = NULL; lkdtm_crashtype = NULL; -- 2.15.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] lkdtm: fix irq handler entry for arm64 2018-02-01 9:34 ` [PATCH 2/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro @ 2018-02-27 3:57 ` Kees Cook 2018-02-27 5:07 ` Masami Hiramatsu 2018-02-27 7:20 ` AKASHI Takahiro 0 siblings, 2 replies; 13+ messages in thread From: Kees Cook @ 2018-02-27 3:57 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 1, 2018 at 1:34 AM, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Arm64 doesn't have "do_IRQ" function, instead *handle_arch_irq, which is > initialized by irq chip (gic), is called from exception entry. > This patch fixes this problem. As in, this symbol is not known a lkdtm setup time? Hm, seems like we'd want a more generalized approach here. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > drivers/misc/lkdtm_core.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c > index ba92291508dc..e20343543053 100644 > --- a/drivers/misc/lkdtm_core.c > +++ b/drivers/misc/lkdtm_core.c > @@ -249,13 +249,29 @@ static int lkdtm_register_cpoint(struct crashpoint *crashpoint, > if (lkdtm_kprobe != NULL) > unregister_kprobe(lkdtm_kprobe); > > + if (IS_ENABLED(CONFIG_ARM64) && > + !strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { > + extern void (*handle_arch_irq)(struct pt_regs *regs); I don't like this extern -- can handle_arch_irq be properly exported somewhere? > + crashpoint->kprobe.addr = (kprobe_opcode_t *)*handle_arch_irq; I don't think the * is needed here: it's already a function pointer. > + /* > + * Instantiating kprobe.symbol_name here, say > + * with lookup_symbol_name(*handle_arch_irq), > + * would cause register_kprobe() to fail. > + */ > + crashpoint->kprobe.symbol_name = NULL; Is kprobe.addr sufficient for register_kprobe? > + } > lkdtm_crashpoint = crashpoint; > lkdtm_crashtype = crashtype; > lkdtm_kprobe = &crashpoint->kprobe; > ret = register_kprobe(lkdtm_kprobe); > if (ret < 0) { > - pr_info("Couldn't register kprobe %s\n", > - crashpoint->kprobe.symbol_name); > + if (IS_ENABLED(CONFIG_ARM64)) > + pr_info("Couldn't register kprobe 0x%lx\n", > + (unsigned long)crashpoint->kprobe.addr); > + else > + pr_info("Couldn't register kprobe %s\n", > + crashpoint->kprobe.symbol_name); > lkdtm_kprobe = NULL; > lkdtm_crashpoint = NULL; > lkdtm_crashtype = NULL; So I can replicate, how did you test this? -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] lkdtm: fix irq handler entry for arm64 2018-02-27 3:57 ` Kees Cook @ 2018-02-27 5:07 ` Masami Hiramatsu 2018-02-27 7:20 ` AKASHI Takahiro 1 sibling, 0 replies; 13+ messages in thread From: Masami Hiramatsu @ 2018-02-27 5:07 UTC (permalink / raw) To: linux-arm-kernel Hi, 2018-02-27 12:57 GMT+09:00 Kees Cook <keescook@chromium.org>: > On Thu, Feb 1, 2018 at 1:34 AM, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: >> Arm64 doesn't have "do_IRQ" function, instead *handle_arch_irq, which is >> initialized by irq chip (gic), is called from exception entry. >> This patch fixes this problem. > > As in, this symbol is not known a lkdtm setup time? Hm, seems like > we'd want a more generalized approach here. > >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> drivers/misc/lkdtm_core.c | 20 ++++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c >> index ba92291508dc..e20343543053 100644 >> --- a/drivers/misc/lkdtm_core.c >> +++ b/drivers/misc/lkdtm_core.c >> @@ -249,13 +249,29 @@ static int lkdtm_register_cpoint(struct crashpoint *crashpoint, >> if (lkdtm_kprobe != NULL) >> unregister_kprobe(lkdtm_kprobe); >> >> + if (IS_ENABLED(CONFIG_ARM64) && >> + !strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { >> + extern void (*handle_arch_irq)(struct pt_regs *regs); > > I don't like this extern -- can handle_arch_irq be properly exported somewhere? > >> + crashpoint->kprobe.addr = (kprobe_opcode_t *)*handle_arch_irq; > > I don't think the * is needed here: it's already a function pointer. Since the addr is no void *, gcc warns this assignment from incompatible pointer type. Hmm, maybe better casting it to void *. > >> + /* >> + * Instantiating kprobe.symbol_name here, say >> + * with lookup_symbol_name(*handle_arch_irq), >> + * would cause register_kprobe() to fail. >> + */ >> + crashpoint->kprobe.symbol_name = NULL; > > Is kprobe.addr sufficient for register_kprobe? Yes, if symbol_name is NULL, register_kprobe uses only kprobe.addr to find the probe point. Thank you, -- Masami Hiramatsu ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] lkdtm: fix irq handler entry for arm64 2018-02-27 3:57 ` Kees Cook 2018-02-27 5:07 ` Masami Hiramatsu @ 2018-02-27 7:20 ` AKASHI Takahiro 2018-02-27 15:46 ` Kees Cook 1 sibling, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2018-02-27 7:20 UTC (permalink / raw) To: linux-arm-kernel Hi Kees, On Mon, Feb 26, 2018 at 07:57:10PM -0800, Kees Cook wrote: > On Thu, Feb 1, 2018 at 1:34 AM, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > Arm64 doesn't have "do_IRQ" function, instead *handle_arch_irq, which is > > initialized by irq chip (gic), is called from exception entry. > > This patch fixes this problem. > > As in, this symbol is not known a lkdtm setup time? Hm, seems like > we'd want a more generalized approach here. Hmm. See my comments below. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/misc/lkdtm_core.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c > > index ba92291508dc..e20343543053 100644 > > --- a/drivers/misc/lkdtm_core.c > > +++ b/drivers/misc/lkdtm_core.c > > @@ -249,13 +249,29 @@ static int lkdtm_register_cpoint(struct crashpoint *crashpoint, > > if (lkdtm_kprobe != NULL) > > unregister_kprobe(lkdtm_kprobe); > > > > + if (IS_ENABLED(CONFIG_ARM64) && > > + !strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { > > + extern void (*handle_arch_irq)(struct pt_regs *regs); > > I don't like this extern -- can handle_arch_irq be properly exported somewhere? Define a weak function, get_handle_irq(), in linux/irq.h and a real one in arch code. Then if (!kallsyms_lookup_name(crashpoint->symbol_name)) { if (!strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { func = get_handle_irq(); if (func) { crashpoint->kprobe.addr = func; crashpoint->kprobe.symbol_name = NULL; } else { /* error */ } } /* anything else? */ } Do you like this code better? > > > + crashpoint->kprobe.addr = (kprobe_opcode_t *)*handle_arch_irq; > > I don't think the * is needed here: it's already a function pointer. Will check. > > + /* > > + * Instantiating kprobe.symbol_name here, say > > + * with lookup_symbol_name(*handle_arch_irq), > > + * would cause register_kprobe() to fail. > > + */ > > + crashpoint->kprobe.symbol_name = NULL; > > Is kprobe.addr sufficient for register_kprobe? Yes as Masami explained. Leaving symbol_name ends up failure of register_kprobe(). > > + } > > lkdtm_crashpoint = crashpoint; > > lkdtm_crashtype = crashtype; > > lkdtm_kprobe = &crashpoint->kprobe; > > ret = register_kprobe(lkdtm_kprobe); > > if (ret < 0) { > > - pr_info("Couldn't register kprobe %s\n", > > - crashpoint->kprobe.symbol_name); > > + if (IS_ENABLED(CONFIG_ARM64)) > > + pr_info("Couldn't register kprobe 0x%lx\n", > > + (unsigned long)crashpoint->kprobe.addr); > > + else > > + pr_info("Couldn't register kprobe %s\n", > > + crashpoint->kprobe.symbol_name); > > lkdtm_kprobe = NULL; > > lkdtm_crashpoint = NULL; > > lkdtm_crashtype = NULL; > > So I can replicate, how did you test this? All what I did in my arm64 test is # echo PANIC > /sys/kernel/debug/provoke-crash/INT_HARDWARE_ENTRY The probe point will hit sooner or later and we will see a panic (and kdump kicks in). Thanks, -Takahiro AKASHI > > -- > Kees Cook > Pixel Security ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] lkdtm: fix irq handler entry for arm64 2018-02-27 7:20 ` AKASHI Takahiro @ 2018-02-27 15:46 ` Kees Cook 0 siblings, 0 replies; 13+ messages in thread From: Kees Cook @ 2018-02-27 15:46 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 26, 2018 at 11:20 PM, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Hi Kees, > > On Mon, Feb 26, 2018 at 07:57:10PM -0800, Kees Cook wrote: >> On Thu, Feb 1, 2018 at 1:34 AM, AKASHI Takahiro >> <takahiro.akashi@linaro.org> wrote: >> > Arm64 doesn't have "do_IRQ" function, instead *handle_arch_irq, which is >> > initialized by irq chip (gic), is called from exception entry. >> > This patch fixes this problem. >> >> As in, this symbol is not known a lkdtm setup time? Hm, seems like >> we'd want a more generalized approach here. > > Hmm. See my comments below. > >> > >> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> > --- >> > drivers/misc/lkdtm_core.c | 20 ++++++++++++++++++-- >> > 1 file changed, 18 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c >> > index ba92291508dc..e20343543053 100644 >> > --- a/drivers/misc/lkdtm_core.c >> > +++ b/drivers/misc/lkdtm_core.c >> > @@ -249,13 +249,29 @@ static int lkdtm_register_cpoint(struct crashpoint *crashpoint, >> > if (lkdtm_kprobe != NULL) >> > unregister_kprobe(lkdtm_kprobe); >> > >> > + if (IS_ENABLED(CONFIG_ARM64) && >> > + !strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { >> > + extern void (*handle_arch_irq)(struct pt_regs *regs); >> >> I don't like this extern -- can handle_arch_irq be properly exported somewhere? > > Define a weak function, get_handle_irq(), in linux/irq.h and > a real one in arch code. Then > > if (!kallsyms_lookup_name(crashpoint->symbol_name)) { > if (!strcmp(crashpoint->name, "INT_HARDWARE_ENTRY")) { > func = get_handle_irq(); > if (func) { > crashpoint->kprobe.addr = func; > crashpoint->kprobe.symbol_name = NULL; > } else { > /* error */ > } > } /* anything else? */ > } > > Do you like this code better? Yeah, this is more generalized; thanks! If we end up with other late-defined functions we can further generalize this, but this is fine for our one case. :) > >> >> > + crashpoint->kprobe.addr = (kprobe_opcode_t *)*handle_arch_irq; >> >> I don't think the * is needed here: it's already a function pointer. > > Will check. > >> > + /* >> > + * Instantiating kprobe.symbol_name here, say >> > + * with lookup_symbol_name(*handle_arch_irq), >> > + * would cause register_kprobe() to fail. >> > + */ >> > + crashpoint->kprobe.symbol_name = NULL; >> >> Is kprobe.addr sufficient for register_kprobe? > > Yes as Masami explained. > Leaving symbol_name ends up failure of register_kprobe(). > >> > + } >> > lkdtm_crashpoint = crashpoint; >> > lkdtm_crashtype = crashtype; >> > lkdtm_kprobe = &crashpoint->kprobe; >> > ret = register_kprobe(lkdtm_kprobe); >> > if (ret < 0) { >> > - pr_info("Couldn't register kprobe %s\n", >> > - crashpoint->kprobe.symbol_name); >> > + if (IS_ENABLED(CONFIG_ARM64)) >> > + pr_info("Couldn't register kprobe 0x%lx\n", >> > + (unsigned long)crashpoint->kprobe.addr); >> > + else >> > + pr_info("Couldn't register kprobe %s\n", >> > + crashpoint->kprobe.symbol_name); >> > lkdtm_kprobe = NULL; >> > lkdtm_crashpoint = NULL; >> > lkdtm_crashtype = NULL; >> >> So I can replicate, how did you test this? > > All what I did in my arm64 test is > # echo PANIC > /sys/kernel/debug/provoke-crash/INT_HARDWARE_ENTRY > > The probe point will hit sooner or later and we will see a panic > (and kdump kicks in). Great, thanks! -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-02-27 15:46 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-01 9:34 [PATCH 0/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro 2018-02-01 9:34 ` [PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check AKASHI Takahiro 2018-02-06 14:36 ` Will Deacon 2018-02-07 0:02 ` Masami Hiramatsu 2018-02-15 2:08 ` David Long 2018-02-15 6:47 ` Masami Hiramatsu 2018-02-22 5:19 ` David Long 2018-02-22 5:45 ` Masami Hiramatsu 2018-02-01 9:34 ` [PATCH 2/2] lkdtm: fix irq handler entry for arm64 AKASHI Takahiro 2018-02-27 3:57 ` Kees Cook 2018-02-27 5:07 ` Masami Hiramatsu 2018-02-27 7:20 ` AKASHI Takahiro 2018-02-27 15:46 ` Kees Cook
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.