[PATCH 0/2] lkdtm: fix irq handler entry for arm64

[PATCH 0/2] lkdtm: fix irq handler entry for arm64

[AKASHI Takahiro]

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

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[AKASHI Takahiro]

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

[PATCH 2/2] lkdtm: fix irq handler entry for arm64

[AKASHI Takahiro]

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

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[Will Deacon]

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

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[Masami Hiramatsu]

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

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[David Long]

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

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[Masami Hiramatsu]

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>

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[David Long]

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

[PATCH 1/2] arm64: kprobes: Remove unneeded address sanity check

[Masami Hiramatsu]

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

[PATCH 2/2] lkdtm: fix irq handler entry for arm64

[Kees Cook]

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

[PATCH 2/2] lkdtm: fix irq handler entry for arm64

[Masami Hiramatsu]

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

[PATCH 2/2] lkdtm: fix irq handler entry for arm64

[AKASHI Takahiro]

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

[PATCH 2/2] lkdtm: fix irq handler entry for arm64

[Kees Cook]

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