Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] arm64: kprobes: Fix blacklist checking on arm64
@ 2018-12-17  6:40 Masami Hiramatsu
  2018-12-17  6:40 ` [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe() Masami Hiramatsu
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2018-12-17  6:40 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Pratyush Anand, linux-kernel, Masami Hiramatsu, linux-arm-kernel,
	David A . Long

Hello,

Here is a short series about fixing kprobe blacklist checking on
arm64.

I found that some blacklist checking code were mis-placed in
arch_prepare_kprobe() and arch_within_kprobe_blacklist().
Some sub-function (instruction-level) accept/decline check
should be done in arch_prepare_kprobe() and that should not
be in the blacklist. Also, all function (symbol) level check
must be done by blacklist.

For arm64, it checks the extable entry address in blacklist
and exception/irqentry function in arch_prepare_kprobe().
Moreover, RODATA check is unneeded since kernel/kprobes.c
already ensures the probe address is in kernel-text area.

Thank you,

---

Masami Hiramatsu (3):
      arm64: kprobes: Move extable address check into arch_prepare_kprobe()
      arm64: kprobes: Remove unneeded RODATA check
      arm64: kprobes: Move exception_text check in blacklist


 arch/arm64/kernel/probes/kprobes.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

--
Signature

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2018-12-17  6:40 [PATCH 0/3] arm64: kprobes: Fix blacklist checking on arm64 Masami Hiramatsu
@ 2018-12-17  6:40 ` Masami Hiramatsu
  2019-01-03 17:05   ` James Morse
  2018-12-17  6:41 ` [PATCH 2/3] arm64: kprobes: Remove unneeded RODATA check Masami Hiramatsu
  2018-12-17  6:41 ` [PATCH 3/3] arm64: kprobes: Move exception_text check in blacklist Masami Hiramatsu
  2 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2018-12-17  6:40 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Pratyush Anand, linux-kernel, Masami Hiramatsu, linux-arm-kernel,
	David A . Long

Move extable address check into arch_prepare_kprobe() from
arch_within_kprobe_blacklist().
Please do not blacklisting instructions on exception_table,
since it is a kind of architectural unsupported instruction.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 2a5b338b2542..b2d4b7428ebc 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -102,6 +102,10 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 
 	if (in_exception_text(probe_addr))
 		return -EINVAL;
+
+	if (search_exception_tables(probe_addr))
+		return -EINVAL;
+
 	if (probe_addr >= (unsigned long) __start_rodata &&
 	    probe_addr <= (unsigned long) __end_rodata)
 		return -EINVAL;
@@ -477,8 +481,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
 	    (addr >= (unsigned long)__entry_text_start &&
 	    addr < (unsigned long)__entry_text_end) ||
 	    (addr >= (unsigned long)__idmap_text_start &&
-	    addr < (unsigned long)__idmap_text_end) ||
-	    !!search_exception_tables(addr))
+	    addr < (unsigned long)__idmap_text_end))
 		return true;
 
 	if (!is_kernel_in_hyp_mode()) {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] arm64: kprobes: Remove unneeded RODATA check
  2018-12-17  6:40 [PATCH 0/3] arm64: kprobes: Fix blacklist checking on arm64 Masami Hiramatsu
  2018-12-17  6:40 ` [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe() Masami Hiramatsu
@ 2018-12-17  6:41 ` Masami Hiramatsu
  2019-01-03 17:07   ` James Morse
  2018-12-17  6:41 ` [PATCH 3/3] arm64: kprobes: Move exception_text check in blacklist Masami Hiramatsu
  2 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2018-12-17  6:41 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Pratyush Anand, linux-kernel, Masami Hiramatsu, linux-arm-kernel,
	David A . Long

Remove unneeded RODATA check from arch_prepare_kprobe().

Since check_kprobe_address_safe() already ensured that
the probe address is in kernel text, we don't need to
check whether the address in RODATA or not. That must
be always false.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index b2d4b7428ebc..1dae500d0a81 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -91,8 +91,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;
@@ -106,10 +104,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	if (search_exception_tables(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 */


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/3] arm64: kprobes: Move exception_text check in blacklist
  2018-12-17  6:40 [PATCH 0/3] arm64: kprobes: Fix blacklist checking on arm64 Masami Hiramatsu
  2018-12-17  6:40 ` [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe() Masami Hiramatsu
  2018-12-17  6:41 ` [PATCH 2/3] arm64: kprobes: Remove unneeded RODATA check Masami Hiramatsu
@ 2018-12-17  6:41 ` Masami Hiramatsu
  2 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2018-12-17  6:41 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Pratyush Anand, linux-kernel, Masami Hiramatsu, linux-arm-kernel,
	David A . Long

Move exception/irqentry text address check in blacklist,
since those are symbol based rejection.

If we prohibit probing on the symbols in exception_text,
those should be blacklisted.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 1dae500d0a81..b9e9758b6534 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -98,9 +98,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 (search_exception_tables(probe_addr))
 		return -EINVAL;
 
@@ -475,7 +472,8 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
 	    (addr >= (unsigned long)__entry_text_start &&
 	    addr < (unsigned long)__entry_text_end) ||
 	    (addr >= (unsigned long)__idmap_text_start &&
-	    addr < (unsigned long)__idmap_text_end))
+	    addr < (unsigned long)__idmap_text_end) ||
+	    in_exception_text(addr))
 		return true;
 
 	if (!is_kernel_in_hyp_mode()) {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2018-12-17  6:40 ` [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe() Masami Hiramatsu
@ 2019-01-03 17:05   ` James Morse
  2019-01-08  2:39     ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: James Morse @ 2019-01-03 17:05 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

Hi!

On 17/12/2018 06:40, Masami Hiramatsu wrote:
> Move extable address check into arch_prepare_kprobe() from
> arch_within_kprobe_blacklist().

I'm trying to work out the pattern for what should go in the blacklist, and what
should be rejected by the arch code.

It seems address-ranges should be blacklisted as the contents don't matter.
easy-example: the idmap text.

The arch code should also reject instructions that can't be probed from
arch_prepare_kprobe(). easy-example: exclusive load or store.


> Please do not blacklisting instructions on exception_table,
> since it is a kind of architectural unsupported instruction.

This doesn't fit the pattern, ... what should it be?

The instructions in the exception_table don't matter, its the address that
indicates there is a fixup for page-faults that occur here. We don't need to
look at the instruction to determine this, why can't we treated these as a
blacklisted range?


Thanks,

James


> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 2a5b338b2542..b2d4b7428ebc 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -102,6 +102,10 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  
>  	if (in_exception_text(probe_addr))
>  		return -EINVAL;
> +
> +	if (search_exception_tables(probe_addr))
> +		return -EINVAL;
> +
>  	if (probe_addr >= (unsigned long) __start_rodata &&
>  	    probe_addr <= (unsigned long) __end_rodata)
>  		return -EINVAL;
> @@ -477,8 +481,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
>  	    (addr >= (unsigned long)__entry_text_start &&
>  	    addr < (unsigned long)__entry_text_end) ||
>  	    (addr >= (unsigned long)__idmap_text_start &&
> -	    addr < (unsigned long)__idmap_text_end) ||
> -	    !!search_exception_tables(addr))
> +	    addr < (unsigned long)__idmap_text_end))
>  		return true;
>  
>  	if (!is_kernel_in_hyp_mode()) {
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] arm64: kprobes: Remove unneeded RODATA check
  2018-12-17  6:41 ` [PATCH 2/3] arm64: kprobes: Remove unneeded RODATA check Masami Hiramatsu
@ 2019-01-03 17:07   ` James Morse
  0 siblings, 0 replies; 11+ messages in thread
From: James Morse @ 2019-01-03 17:07 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

Hi!

On 17/12/2018 06:41, Masami Hiramatsu wrote:
> Remove unneeded RODATA check from arch_prepare_kprobe().
> 
> Since check_kprobe_address_safe() already ensured that
> the probe address is in kernel text, we don't need to
> check whether the address in RODATA or not. That must
> be always false.

So it does!

Reviewed-by: James Morse <james.morse@arm.com>


Thanks

James

> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index b2d4b7428ebc..1dae500d0a81 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -91,8 +91,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;
> @@ -106,10 +104,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  	if (search_exception_tables(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 */
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2019-01-03 17:05   ` James Morse
@ 2019-01-08  2:39     ` Masami Hiramatsu
  2019-01-08 17:13       ` James Morse
  0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2019-01-08  2:39 UTC (permalink / raw)
  To: James Morse
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

On Thu, 3 Jan 2019 17:05:18 +0000
James Morse <james.morse@arm.com> wrote:

> Hi!
> 
> On 17/12/2018 06:40, Masami Hiramatsu wrote:
> > Move extable address check into arch_prepare_kprobe() from
> > arch_within_kprobe_blacklist().
> 
> I'm trying to work out the pattern for what should go in the blacklist, and what
> should be rejected by the arch code.
> 
> It seems address-ranges should be blacklisted as the contents don't matter.
> easy-example: the idmap text.

Yes, more precisely, the code smaller than a function (symbol), it must be
rejected by arch_prepare_kprobe(), since blacklist is poplated based on
kallsyms.

> The arch code should also reject instructions that can't be probed from
> arch_prepare_kprobe(). easy-example: exclusive load or store.
> 
> 
> > Please do not blacklisting instructions on exception_table,
> > since it is a kind of architectural unsupported instruction.
> 
> This doesn't fit the pattern, ... what should it be?

Some kind of instructions can not be instrumented by kprobes, such instruction
level rejection must be done in arch_prepare_kprobe(), instead of blacklist.

> The instructions in the exception_table don't matter, its the address that
> indicates there is a fixup for page-faults that occur here. We don't need to
> look at the instruction to determine this, why can't we treated these as a
> blacklisted range?

Sorry for your confusion, it was my mis-describing.

As I pointed, the exception_table contains some range of code which inside
functions, must be smaller than function.
Since those instructions are expected to cause exception (that is main reason
why it can not be probed on arm64), I thought such situation was similar to
the limitation of instruction.

So I think below will be better.
----
Please do not blacklisting instructions on exception_table,
since those are smaller than one function.
----


Thank you,

> 
> 
> Thanks,
> 
> James
> 
> 
> > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > index 2a5b338b2542..b2d4b7428ebc 100644
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -102,6 +102,10 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> >  
> >  	if (in_exception_text(probe_addr))
> >  		return -EINVAL;
> > +
> > +	if (search_exception_tables(probe_addr))
> > +		return -EINVAL;
> > +
> >  	if (probe_addr >= (unsigned long) __start_rodata &&
> >  	    probe_addr <= (unsigned long) __end_rodata)
> >  		return -EINVAL;
> > @@ -477,8 +481,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
> >  	    (addr >= (unsigned long)__entry_text_start &&
> >  	    addr < (unsigned long)__entry_text_end) ||
> >  	    (addr >= (unsigned long)__idmap_text_start &&
> > -	    addr < (unsigned long)__idmap_text_end) ||
> > -	    !!search_exception_tables(addr))
> > +	    addr < (unsigned long)__idmap_text_end))
> >  		return true;
> >  
> >  	if (!is_kernel_in_hyp_mode()) {
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2019-01-08  2:39     ` Masami Hiramatsu
@ 2019-01-08 17:13       ` James Morse
  2019-01-09  2:05         ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: James Morse @ 2019-01-08 17:13 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

Hi!

On 08/01/2019 02:39, Masami Hiramatsu wrote:
> On Thu, 3 Jan 2019 17:05:18 +0000
> James Morse <james.morse@arm.com> wrote:
>> On 17/12/2018 06:40, Masami Hiramatsu wrote:
>>> Move extable address check into arch_prepare_kprobe() from
>>> arch_within_kprobe_blacklist().
>>
>> I'm trying to work out the pattern for what should go in the blacklist, and what
>> should be rejected by the arch code.
>>
>> It seems address-ranges should be blacklisted as the contents don't matter.
>> easy-example: the idmap text.
> 
> Yes, more precisely, the code smaller than a function (symbol), it must be
> rejected by arch_prepare_kprobe(), since blacklist is poplated based on
> kallsyms.

Ah, okay, so the pattern is the blacklist should only be for whole symbols,
(which explains why its usually based on sections).

I see kprobe_add_ksym_blacklist() would go wrong if you give it something like:
platform_drv_probe+0x50/0xb0, as it will log platform_drv_probe+0x50 as the
start_addr and platform_drv_probe+0x50+0xb0 as the end.

But how does anything from the arch code's blacklist get into the
kprobe_blacklist list?

We don't have an arch_populate_kprobe_blacklist(), so rely on
within_kprobe_blacklist() calling arch_within_kprobe_blacklist() with the
address, as well as walking kprobe_blacklist.

Is this cleanup ahead of a series that does away with
arch_within_kprobe_blacklist() so that debugfs list is always complete?


> As I pointed, the exception_table contains some range of code which inside
> functions, must be smaller than function.
> Since those instructions are expected to cause exception (that is main reason
> why it can not be probed on arm64), I thought such situation was similar to
> the limitation of instruction.
> 
> So I think below will be better.
> ----
> Please do not blacklisting instructions on exception_table,
> since those are smaller than one function.
> ----

I keep tripping over this because the exception_table lists addresses that are
allowed to fault. Nothing looks at the instruction, and we happily kprobe the
same instruction elsewhere.

(based on my assumptions about where you are going next!,), How about:
| The blacklist is exposed via debugfs as a list of symbols. extable entries are
| smaller, so must be filtered out by arch_prepare_kprobe().

(only we currently have more than one blacklist...)


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2019-01-08 17:13       ` James Morse
@ 2019-01-09  2:05         ` Masami Hiramatsu
  2019-01-11 18:22           ` James Morse
  0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2019-01-09  2:05 UTC (permalink / raw)
  To: James Morse
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

Hi James,

On Tue, 8 Jan 2019 17:13:36 +0000
James Morse <james.morse@arm.com> wrote:

> Hi!
> 
> On 08/01/2019 02:39, Masami Hiramatsu wrote:
> > On Thu, 3 Jan 2019 17:05:18 +0000
> > James Morse <james.morse@arm.com> wrote:
> >> On 17/12/2018 06:40, Masami Hiramatsu wrote:
> >>> Move extable address check into arch_prepare_kprobe() from
> >>> arch_within_kprobe_blacklist().
> >>
> >> I'm trying to work out the pattern for what should go in the blacklist, and what
> >> should be rejected by the arch code.
> >>
> >> It seems address-ranges should be blacklisted as the contents don't matter.
> >> easy-example: the idmap text.
> > 
> > Yes, more precisely, the code smaller than a function (symbol), it must be
> > rejected by arch_prepare_kprobe(), since blacklist is poplated based on
> > kallsyms.
> 
> Ah, okay, so the pattern is the blacklist should only be for whole symbols,
> (which explains why its usually based on sections).

Correct. Actually, the blacklist is generated based on the symbol info
from symbol address.

> I see kprobe_add_ksym_blacklist() would go wrong if you give it something like:
> platform_drv_probe+0x50/0xb0, as it will log platform_drv_probe+0x50 as the
> start_addr and platform_drv_probe+0x50+0xb0 as the end.

Yes, it expects given address is the entry of a symbol.

> 
> But how does anything from the arch code's blacklist get into the
> kprobe_blacklist list?

It should be done via arch_populate_kprobe_blacklist().

> 
> We don't have an arch_populate_kprobe_blacklist(), so rely on
> within_kprobe_blacklist() calling arch_within_kprobe_blacklist() with the
> address, as well as walking kprobe_blacklist.
> 
> Is this cleanup ahead of a series that does away with
> arch_within_kprobe_blacklist() so that debugfs list is always complete?

Right, after this cleanup, I will send arch_populate_kprobe_blacklist()
patch for arm64 and others. My plan is to move all arch_within_kprobe_blacklist()
to arch_populate_kprobe_blacklist() so that user can get more precise blacklist
via debugfs.


> > As I pointed, the exception_table contains some range of code which inside
> > functions, must be smaller than function.
> > Since those instructions are expected to cause exception (that is main reason
> > why it can not be probed on arm64), I thought such situation was similar to
> > the limitation of instruction.
> > 
> > So I think below will be better.
> > ----
> > Please do not blacklisting instructions on exception_table,
> > since those are smaller than one function.
> > ----
> 
> I keep tripping over this because the exception_table lists addresses that are
> allowed to fault. Nothing looks at the instruction, and we happily kprobe the
> same instruction elsewhere.

Thanks!

> 
> (based on my assumptions about where you are going next!,), How about:
> | The blacklist is exposed via debugfs as a list of symbols. extable entries are
> | smaller, so must be filtered out by arch_prepare_kprobe().

This looks much better for me too :)
Should I resend with the description?

Thank you!

> 
> (only we currently have more than one blacklist...)
> 
> 
> Thanks,
> 
> James


-- 
Masami Hiramatsu <mhiramat@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2019-01-09  2:05         ` Masami Hiramatsu
@ 2019-01-11 18:22           ` James Morse
  2019-01-15  5:49             ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: James Morse @ 2019-01-11 18:22 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

Hi,

On 09/01/2019 02:05, Masami Hiramatsu wrote:
> On Tue, 8 Jan 2019 17:13:36 +0000
> James Morse <james.morse@arm.com> wrote:
>> On 08/01/2019 02:39, Masami Hiramatsu wrote:
>>> On Thu, 3 Jan 2019 17:05:18 +0000
>>> James Morse <james.morse@arm.com> wrote:
>>>> On 17/12/2018 06:40, Masami Hiramatsu wrote:
>>>>> Move extable address check into arch_prepare_kprobe() from
>>>>> arch_within_kprobe_blacklist().
>>>>
>>>> I'm trying to work out the pattern for what should go in the blacklist, and what
>>>> should be rejected by the arch code.
>>>>
>>>> It seems address-ranges should be blacklisted as the contents don't matter.
>>>> easy-example: the idmap text.
>>>
>>> Yes, more precisely, the code smaller than a function (symbol), it must be
>>> rejected by arch_prepare_kprobe(), since blacklist is poplated based on
>>> kallsyms.
>>
>> Ah, okay, so the pattern is the blacklist should only be for whole symbols,
>> (which explains why its usually based on sections).
> 
> Correct. Actually, the blacklist is generated based on the symbol info
> from symbol address.
> 
>> I see kprobe_add_ksym_blacklist() would go wrong if you give it something like:
>> platform_drv_probe+0x50/0xb0, as it will log platform_drv_probe+0x50 as the
>> start_addr and platform_drv_probe+0x50+0xb0 as the end.
> 
> Yes, it expects given address is the entry of a symbol.

>> But how does anything from the arch code's blacklist get into the
>> kprobe_blacklist list?
> 
> It should be done via arch_populate_kprobe_blacklist().

>> We don't have an arch_populate_kprobe_blacklist(), so rely on
>> within_kprobe_blacklist() calling arch_within_kprobe_blacklist() with the
>> address, as well as walking kprobe_blacklist.
>>
>> Is this cleanup ahead of a series that does away with
>> arch_within_kprobe_blacklist() so that debugfs list is always complete?
> 
> Right, after this cleanup, I will send arch_populate_kprobe_blacklist()
> patch for arm64 and others. My plan is to move all arch_within_kprobe_blacklist()
> to arch_populate_kprobe_blacklist() so that user can get more precise blacklist
> via debugfs.

Thanks, now it all makes sense!

Reviewed-by: James Morse <james.morse@arm.com>


Could you include a paragraph like that in the cover-letter or commit-message?
The 'fix' in the cover-letter subject had me looking for the bug!


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()
  2019-01-11 18:22           ` James Morse
@ 2019-01-15  5:49             ` Masami Hiramatsu
  0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2019-01-15  5:49 UTC (permalink / raw)
  To: James Morse
  Cc: Pratyush Anand, Catalin Marinas, Will Deacon, linux-kernel,
	David A . Long, linux-arm-kernel

On Fri, 11 Jan 2019 18:22:38 +0000
James Morse <james.morse@arm.com> wrote:

> Hi,
> 
> On 09/01/2019 02:05, Masami Hiramatsu wrote:
> > On Tue, 8 Jan 2019 17:13:36 +0000
> > James Morse <james.morse@arm.com> wrote:
> >> On 08/01/2019 02:39, Masami Hiramatsu wrote:
> >>> On Thu, 3 Jan 2019 17:05:18 +0000
> >>> James Morse <james.morse@arm.com> wrote:
> >>>> On 17/12/2018 06:40, Masami Hiramatsu wrote:
> >>>>> Move extable address check into arch_prepare_kprobe() from
> >>>>> arch_within_kprobe_blacklist().
> >>>>
> >>>> I'm trying to work out the pattern for what should go in the blacklist, and what
> >>>> should be rejected by the arch code.
> >>>>
> >>>> It seems address-ranges should be blacklisted as the contents don't matter.
> >>>> easy-example: the idmap text.
> >>>
> >>> Yes, more precisely, the code smaller than a function (symbol), it must be
> >>> rejected by arch_prepare_kprobe(), since blacklist is poplated based on
> >>> kallsyms.
> >>
> >> Ah, okay, so the pattern is the blacklist should only be for whole symbols,
> >> (which explains why its usually based on sections).
> > 
> > Correct. Actually, the blacklist is generated based on the symbol info
> > from symbol address.
> > 
> >> I see kprobe_add_ksym_blacklist() would go wrong if you give it something like:
> >> platform_drv_probe+0x50/0xb0, as it will log platform_drv_probe+0x50 as the
> >> start_addr and platform_drv_probe+0x50+0xb0 as the end.
> > 
> > Yes, it expects given address is the entry of a symbol.
> 
> >> But how does anything from the arch code's blacklist get into the
> >> kprobe_blacklist list?
> > 
> > It should be done via arch_populate_kprobe_blacklist().
> 
> >> We don't have an arch_populate_kprobe_blacklist(), so rely on
> >> within_kprobe_blacklist() calling arch_within_kprobe_blacklist() with the
> >> address, as well as walking kprobe_blacklist.
> >>
> >> Is this cleanup ahead of a series that does away with
> >> arch_within_kprobe_blacklist() so that debugfs list is always complete?
> > 
> > Right, after this cleanup, I will send arch_populate_kprobe_blacklist()
> > patch for arm64 and others. My plan is to move all arch_within_kprobe_blacklist()
> > to arch_populate_kprobe_blacklist() so that user can get more precise blacklist
> > via debugfs.
> 
> Thanks, now it all makes sense!
> 
> Reviewed-by: James Morse <james.morse@arm.com>

Thanks!

> 
> 
> Could you include a paragraph like that in the cover-letter or commit-message?
> The 'fix' in the cover-letter subject had me looking for the bug!

Ok, I'll update commit message with your reviewed-by.

Thank you!

> 
> 
> Thanks,
> 
> James


-- 
Masami Hiramatsu <mhiramat@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-17  6:40 [PATCH 0/3] arm64: kprobes: Fix blacklist checking on arm64 Masami Hiramatsu
2018-12-17  6:40 ` [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe() Masami Hiramatsu
2019-01-03 17:05   ` James Morse
2019-01-08  2:39     ` Masami Hiramatsu
2019-01-08 17:13       ` James Morse
2019-01-09  2:05         ` Masami Hiramatsu
2019-01-11 18:22           ` James Morse
2019-01-15  5:49             ` Masami Hiramatsu
2018-12-17  6:41 ` [PATCH 2/3] arm64: kprobes: Remove unneeded RODATA check Masami Hiramatsu
2019-01-03 17:07   ` James Morse
2018-12-17  6:41 ` [PATCH 3/3] arm64: kprobes: Move exception_text check in blacklist Masami Hiramatsu

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox