[PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3

[PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3

[Naveen N. Rao]

This is the third in the series of patches to build out an appropriate
kprobes blacklist for powerpc. Since posting the second series (*),
there have been related changes to the code and I have brought that
series forward to account for those changes. As such, all patches from
the second series are included in this patchset.

This patchset now ensures that the newly added multiple kprobes test in
the ftrace testsuite passes on powerpc64. Tested on both Elfv1 and
Elfv2.

Changes since v3:
  - Patch 1 now implements a different approach by checking if the function
    pointer points to .text or not.
  - Patch 4 is new, as suggested by Nick.
  - Patch 6 (previously 5) changed to leave 'restore' symbol alone.
  - Patch 7 (previously 6) moves __rtas_enter after the mtmsr.
  - Patches 2, 3 and 5 (previously numbered 4) are unchanged

v3:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119596.html

(*) series 2:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg117562.html


- Naveen


Naveen N. Rao (7):
  powerpc64/elfv1: Only dereference function descriptor for non-text
    symbols
  powerpc/64s: Convert .L__replay_interrupt_return to a local label
  powerpc/64s: Blacklist system_call() and system_call_common() from
    kprobes
  powerpc/64s: Move system_call() symbol to just after setting MSR_EE
  powerpc/64s: Un-blacklist system_call() from kprobes
  powerpc/64s: Blacklist functions invoked on a trap
  powerpc/64s: Blacklist rtas entry/exit from kprobes

 arch/powerpc/include/asm/code-patching.h | 10 ++++-
 arch/powerpc/kernel/entry_64.S           | 75 +++++++++++++++++++-------------
 arch/powerpc/kernel/exceptions-64s.S     |  6 ++-
 arch/powerpc/kernel/traps.c              |  3 ++
 4 files changed, 61 insertions(+), 33 deletions(-)

-- 
2.13.1

[PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols

[Naveen N. Rao]

Currently, we assume that the function pointer we receive in
ppc_function_entry() points to a function descriptor. However, this is
not always the case. In particular, assembly symbols without the right
annotation do not have an associated function descriptor. Some of these
symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().

When such addresses are subsequently processed through
arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
below errors during bootup:
    [    0.663963] Failed to find blacklist at 7d9b02a648029b6c
    [    0.663970] Failed to find blacklist at a14d03d0394a0001
    [    0.663972] Failed to find blacklist at 7d5302a6f94d0388
    [    0.663973] Failed to find blacklist at 48027d11e8610178
    [    0.663974] Failed to find blacklist at f8010070f8410080
    [    0.663976] Failed to find blacklist at 386100704801f89d
    [    0.663977] Failed to find blacklist at 7d5302a6f94d00b0

Fix this by checking if the function pointer we receive in
ppc_function_entry() already points to kernel text. If so, we just
return it as is. If not, we assume that this is a function descriptor
and proceed to dereference it.

Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..5482928eea1b 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
 	 * On PPC64 ABIv1 the function pointer actually points to the
 	 * function's descriptor. The first entry in the descriptor is the
 	 * address of the function text.
+	 *
+	 * However, we may also receive pointer to an assembly symbol. To
+	 * detect that, we first check if the function pointer we receive
+	 * already points to kernel/module text and we only dereference it
+	 * if it doesn't.
 	 */
-	return ((func_descr_t *)func)->entry;
+	if (kernel_text_address((unsigned long)func))
+		return (unsigned long)func;
+	else
+		return ((func_descr_t *)func)->entry;
 #else
 	return (unsigned long)func;
 #endif
-- 
2.13.1

Re: [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols

[Nicholas Piggin]

On Thu, 29 Jun 2017 16:11:04 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Currently, we assume that the function pointer we receive in
> ppc_function_entry() points to a function descriptor. However, this is
> not always the case. In particular, assembly symbols without the right
> annotation do not have an associated function descriptor. Some of these
> symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
> 
> When such addresses are subsequently processed through
> arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
> below errors during bootup:
>     [    0.663963] Failed to find blacklist at 7d9b02a648029b6c
>     [    0.663970] Failed to find blacklist at a14d03d0394a0001
>     [    0.663972] Failed to find blacklist at 7d5302a6f94d0388
>     [    0.663973] Failed to find blacklist at 48027d11e8610178
>     [    0.663974] Failed to find blacklist at f8010070f8410080
>     [    0.663976] Failed to find blacklist at 386100704801f89d
>     [    0.663977] Failed to find blacklist at 7d5302a6f94d00b0
> 
> Fix this by checking if the function pointer we receive in
> ppc_function_entry() already points to kernel text. If so, we just
> return it as is. If not, we assume that this is a function descriptor
> and proceed to dereference it.
> 
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index abef812de7f8..5482928eea1b 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
>  	 * On PPC64 ABIv1 the function pointer actually points to the
>  	 * function's descriptor. The first entry in the descriptor is the
>  	 * address of the function text.
> +	 *
> +	 * However, we may also receive pointer to an assembly symbol. To
> +	 * detect that, we first check if the function pointer we receive
> +	 * already points to kernel/module text and we only dereference it
> +	 * if it doesn't.
>  	 */
> -	return ((func_descr_t *)func)->entry;
> +	if (kernel_text_address((unsigned long)func))
> +		return (unsigned long)func;
> +	else
> +		return ((func_descr_t *)func)->entry;

This seems good to me now. I guess it should do the right thing with
modules too, looking at kernel_text_address implementation.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Re: [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols

[Naveen N. Rao]

On 2017/06/29 08:49PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 16:11:04 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > Currently, we assume that the function pointer we receive in
> > ppc_function_entry() points to a function descriptor. However, this is
> > not always the case. In particular, assembly symbols without the right
> > annotation do not have an associated function descriptor. Some of these
> > symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
> > 
> > When such addresses are subsequently processed through
> > arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
> > below errors during bootup:
> >     [    0.663963] Failed to find blacklist at 7d9b02a648029b6c
> >     [    0.663970] Failed to find blacklist at a14d03d0394a0001
> >     [    0.663972] Failed to find blacklist at 7d5302a6f94d0388
> >     [    0.663973] Failed to find blacklist at 48027d11e8610178
> >     [    0.663974] Failed to find blacklist at f8010070f8410080
> >     [    0.663976] Failed to find blacklist at 386100704801f89d
> >     [    0.663977] Failed to find blacklist at 7d5302a6f94d00b0
> > 
> > Fix this by checking if the function pointer we receive in
> > ppc_function_entry() already points to kernel text. If so, we just
> > return it as is. If not, we assume that this is a function descriptor
> > and proceed to dereference it.
> > 
> > Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> > index abef812de7f8..5482928eea1b 100644
> > --- a/arch/powerpc/include/asm/code-patching.h
> > +++ b/arch/powerpc/include/asm/code-patching.h
> > @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
> >  	 * On PPC64 ABIv1 the function pointer actually points to the
> >  	 * function's descriptor. The first entry in the descriptor is the
> >  	 * address of the function text.
> > +	 *
> > +	 * However, we may also receive pointer to an assembly symbol. To
> > +	 * detect that, we first check if the function pointer we receive
> > +	 * already points to kernel/module text and we only dereference it
> > +	 * if it doesn't.
> >  	 */
> > -	return ((func_descr_t *)func)->entry;
> > +	if (kernel_text_address((unsigned long)func))
> > +		return (unsigned long)func;
> > +	else
> > +		return ((func_descr_t *)func)->entry;
> 
> This seems good to me now. I guess it should do the right thing with
> modules too, looking at kernel_text_address implementation.

Yes, I've tested that by exercizing the jprobe sample module. We use 
this to lookup the address of the jprobe handler which happens to be in 
the jprobe module and it is working properly:
  $ sudo modprobe jprobe_example
  $ dmesg | grep Planted
  [ 4817.649072] Planted jprobe at c0000000000ed7b0, handler addr d0000000074b03e8
  $ sudo grep j_do_fork /proc/kallsyms
  d0000000074b03e8 d j_do_fork	[jprobe_example]
  d0000000074b0000 t .j_do_fork	[jprobe_example]

> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 

Thanks for the review,
- Naveen

[PATCH v4 2/7] powerpc/64s: Convert .L__replay_interrupt_return to a local label

[Naveen N. Rao]

Commit b48bbb82e2b835 ("powerpc/64s: Don't unbalance the return branch
predictor in __replay_interrupt()") introduced __replay_interrupt_return
symbol with '.L' prefix in hopes of keeping it private. However, due to
the use of LOAD_REG_ADDR(), the assembler kept this symbol visible. Fix
the same by instead using the local label '1'.

Fixes: Commit b48bbb82e2b835 ("powerpc/64s: Don't unbalance the return branch
predictor in __replay_interrupt()")
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 02a82777fd5b..3262e447b895 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1631,7 +1631,7 @@ _GLOBAL(__replay_interrupt)
 	 * we don't give a damn about, so we don't bother storing them.
 	 */
 	mfmsr	r12
-	LOAD_REG_ADDR(r11, .L__replay_interrupt_return)
+	LOAD_REG_ADDR(r11, 1f)
 	mfcr	r9
 	ori	r12,r12,MSR_EE
 	cmpwi	r3,0x900
@@ -1649,6 +1649,6 @@ FTR_SECTION_ELSE
 	cmpwi	r3,0xa00
 	beq	doorbell_super_common_msgclr
 ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
-.L__replay_interrupt_return:
+1:
 	blr
 
-- 
2.13.1

[PATCH v4 3/7] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes

[Naveen N. Rao]

Convert some of the symbols into private symbols and blacklist
system_call_common() and system_call() from kprobes. We can't take a
trap at parts of these functions as either MSR_RI is unset or the kernel
stack pointer is not yet setup.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_64.S | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index da9486e2fd89..ef8e6615b8ba 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -52,12 +52,11 @@ exception_marker:
 	.section	".text"
 	.align 7
 
-	.globl system_call_common
-system_call_common:
+_GLOBAL(system_call_common)
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 BEGIN_FTR_SECTION
 	extrdi.	r10, r12, 1, (63-MSR_TS_T_LG) /* transaction active? */
-	bne	tabort_syscall
+	bne	.Ltabort_syscall
 END_FTR_SECTION_IFSET(CPU_FTR_TM)
 #endif
 	andi.	r10,r12,MSR_PR
@@ -152,9 +151,9 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 	CURRENT_THREAD_INFO(r11, r1)
 	ld	r10,TI_FLAGS(r11)
 	andi.	r11,r10,_TIF_SYSCALL_DOTRACE
-	bne	syscall_dotrace		/* does not return */
+	bne	.Lsyscall_dotrace		/* does not return */
 	cmpldi	0,r0,NR_syscalls
-	bge-	syscall_enosys
+	bge-	.Lsyscall_enosys
 
 system_call:			/* label this so stack traces look sane */
 /*
@@ -208,7 +207,7 @@ system_call:			/* label this so stack traces look sane */
 	ld	r9,TI_FLAGS(r12)
 	li	r11,-MAX_ERRNO
 	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
-	bne-	syscall_exit_work
+	bne-	.Lsyscall_exit_work
 
 	/* If MSR_FP and MSR_VEC are set in user msr, then no need to restore */
 	li	r7,MSR_FP
@@ -217,12 +216,12 @@ system_call:			/* label this so stack traces look sane */
 #endif
 	and	r0,r8,r7
 	cmpd	r0,r7
-	bne	syscall_restore_math
+	bne	.Lsyscall_restore_math
 .Lsyscall_restore_math_cont:
 
 	cmpld	r3,r11
 	ld	r5,_CCR(r1)
-	bge-	syscall_error
+	bge-	.Lsyscall_error
 .Lsyscall_error_cont:
 	ld	r7,_NIP(r1)
 BEGIN_FTR_SECTION
@@ -248,13 +247,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	RFI
 	b	.	/* prevent speculative execution */
 
-syscall_error:	
+.Lsyscall_error:
 	oris	r5,r5,0x1000	/* Set SO bit in CR */
 	neg	r3,r3
 	std	r5,_CCR(r1)
 	b	.Lsyscall_error_cont
 
-syscall_restore_math:
+.Lsyscall_restore_math:
 	/*
 	 * Some initial tests from restore_math to avoid the heavyweight
 	 * C code entry and MSR manipulations.
@@ -289,7 +288,7 @@ syscall_restore_math:
 	b	.Lsyscall_restore_math_cont
 
 /* Traced system call support */
-syscall_dotrace:
+.Lsyscall_dotrace:
 	bl	save_nvgprs
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	do_syscall_trace_enter
@@ -322,11 +321,11 @@ syscall_dotrace:
 	b	.Lsyscall_exit
 
 
-syscall_enosys:
+.Lsyscall_enosys:
 	li	r3,-ENOSYS
 	b	.Lsyscall_exit
 	
-syscall_exit_work:
+.Lsyscall_exit_work:
 #ifdef CONFIG_PPC_BOOK3S
 	li	r10,MSR_RI
 	mtmsrd	r10,1		/* Restore RI */
@@ -386,7 +385,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	b	ret_from_except
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-tabort_syscall:
+.Ltabort_syscall:
 	/* Firstly we need to enable TM in the kernel */
 	mfmsr	r10
 	li	r9, 1
@@ -412,6 +411,8 @@ tabort_syscall:
 	rfid
 	b	.	/* prevent speculative execution */
 #endif
+_ASM_NOKPROBE_SYMBOL(system_call_common);
+_ASM_NOKPROBE_SYMBOL(system_call);
 
 /* Save non-volatile GPRs, if not already saved. */
 _GLOBAL(save_nvgprs)
-- 
2.13.1

[PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE

[Naveen N. Rao]

It is common to get a PMU interrupt right after the mtmsr instruction that
enables interrupts. Due to this, the stack trace profile gets needlessly split
across system_call_common() and system_call().

Previously, system_call() symbol was at the current place to hide a few
earlier symbols which have since been made private or removed entirely.

So, let's move system_call() slightly higher up, right after the mtmsr
instruction that enables interrupts. Convert existing references to
system_call to a local syscall symbol.

Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index ef8e6615b8ba..c39436706555 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -142,6 +142,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */
 
+system_call:			/* label this so stack traces look sane */
 	/* We do need to set SOFTE in the stack frame or the return
 	 * from interrupt will be painful
 	 */
@@ -155,7 +156,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 	cmpldi	0,r0,NR_syscalls
 	bge-	.Lsyscall_enosys
 
-system_call:			/* label this so stack traces look sane */
+.Lsyscall:
 /*
  * Need to vector to 32 Bit or default sys_call_table here,
  * based on caller's run-mode / personality.
@@ -309,13 +310,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	ld	r7,GPR7(r1)
 	ld	r8,GPR8(r1)
 
-	/* Repopulate r9 and r10 for the system_call path */
+	/* Repopulate r9 and r10 for the syscall path */
 	addi	r9,r1,STACK_FRAME_OVERHEAD
 	CURRENT_THREAD_INFO(r10, r1)
 	ld	r10,TI_FLAGS(r10)
 
 	cmpldi	r0,NR_syscalls
-	blt+	system_call
+	blt+	.Lsyscall
 
 	/* Return code is already in r3 thanks to do_syscall_trace_enter() */
 	b	.Lsyscall_exit
-- 
2.13.1

Re: [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE

[Nicholas Piggin]

On Thu, 29 Jun 2017 16:11:07 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> It is common to get a PMU interrupt right after the mtmsr instruction that
> enables interrupts. Due to this, the stack trace profile gets needlessly split
> across system_call_common() and system_call().
> 
> Previously, system_call() symbol was at the current place to hide a few
> earlier symbols which have since been made private or removed entirely.
> 
> So, let's move system_call() slightly higher up, right after the mtmsr
> instruction that enables interrupts. Convert existing references to
> system_call to a local syscall symbol.
> 
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Ah, I didn't realize the label is actually used. Still, I like the
patch myself.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

[PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes

[Naveen N. Rao]

It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_64.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index c39436706555..7a87427a67cd 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -205,6 +205,7 @@ system_call:			/* label this so stack traces look sane */
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */
 
+system_call_exit:
 	ld	r9,TI_FLAGS(r12)
 	li	r11,-MAX_ERRNO
 	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
@@ -413,7 +414,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	b	.	/* prevent speculative execution */
 #endif
 _ASM_NOKPROBE_SYMBOL(system_call_common);
-_ASM_NOKPROBE_SYMBOL(system_call);
+_ASM_NOKPROBE_SYMBOL(system_call_exit);
 
 /* Save non-volatile GPRs, if not already saved. */
 _GLOBAL(save_nvgprs)
-- 
2.13.1

Re: [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes

[Nicholas Piggin]

On Thu, 29 Jun 2017 16:11:08 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> It is actually safe to probe system_call() in entry_64.S, but only till
> we unset MSR_RI. To allow this, add a new symbol system_call_exit()
> after the mtmsrd and blacklist that. Though the mtmsrd instruction
> itself is now whitelisted, we won't be allowed to probe on it as we
> don't allow probing on rfi and mtmsr instructions (checked for in
> arch_prepare_kprobe()).

Can you perhaps add a small comment to explain the label
(and why it's safe to have after the mtmsrd). It could be
a bit confusing to read if you don't have that detail of
the tracer in your mind.

Other than that

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>


> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/entry_64.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index c39436706555..7a87427a67cd 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -205,6 +205,7 @@ system_call:			/* label this so stack traces look sane */
>  	mtmsrd	r11,1
>  #endif /* CONFIG_PPC_BOOK3E */
>  
> +system_call_exit:
>  	ld	r9,TI_FLAGS(r12)
>  	li	r11,-MAX_ERRNO
>  	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
> @@ -413,7 +414,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	b	.	/* prevent speculative execution */
>  #endif
>  _ASM_NOKPROBE_SYMBOL(system_call_common);
> -_ASM_NOKPROBE_SYMBOL(system_call);
> +_ASM_NOKPROBE_SYMBOL(system_call_exit);
>  
>  /* Save non-volatile GPRs, if not already saved. */
>  _GLOBAL(save_nvgprs)

Re: [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes

[Naveen N. Rao]

On 2017/06/29 08:55PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 16:11:08 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > It is actually safe to probe system_call() in entry_64.S, but only till
> > we unset MSR_RI. To allow this, add a new symbol system_call_exit()
> > after the mtmsrd and blacklist that. Though the mtmsrd instruction
> > itself is now whitelisted, we won't be allowed to probe on it as we
> > don't allow probing on rfi and mtmsr instructions (checked for in
> > arch_prepare_kprobe()).
> 
> Can you perhaps add a small comment to explain the label
> (and why it's safe to have after the mtmsrd). It could be
> a bit confusing to read if you don't have that detail of
> the tracer in your mind.

Ah, I see. You did ask for this previously, but I thought you were just 
asking for a comment that this is blacklisted, which can be found out by 
looking for the _ASM_NOKPROBE_SYMBOL() further below.

I will add a comment.

Thanks,
Naveen

> 
> Other than that
> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 
> 
> > 
> > Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/entry_64.S | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index c39436706555..7a87427a67cd 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -205,6 +205,7 @@ system_call:			/* label this so stack traces look sane */
> >  	mtmsrd	r11,1
> >  #endif /* CONFIG_PPC_BOOK3E */
> >  
> > +system_call_exit:
> >  	ld	r9,TI_FLAGS(r12)
> >  	li	r11,-MAX_ERRNO
> >  	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
> > @@ -413,7 +414,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> >  	b	.	/* prevent speculative execution */
> >  #endif
> >  _ASM_NOKPROBE_SYMBOL(system_call_common);
> > -_ASM_NOKPROBE_SYMBOL(system_call);
> > +_ASM_NOKPROBE_SYMBOL(system_call_exit);
> >  
> >  /* Save non-volatile GPRs, if not already saved. */
> >  _GLOBAL(save_nvgprs)
> 

[PATCH v4 6/7] powerpc/64s: Blacklist functions invoked on a trap

[Naveen N. Rao]

Blacklist all functions involved while handling a trap. We:
- convert some of the symbols into private symbols, and
- blacklist most functions involved while handling a trap.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_64.S       | 35 ++++++++++++++++++++++-------------
 arch/powerpc/kernel/exceptions-64s.S |  2 ++
 arch/powerpc/kernel/traps.c          |  3 +++
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 7a87427a67cd..0c27084800b6 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -185,7 +185,7 @@ system_call:			/* label this so stack traces look sane */
 #ifdef CONFIG_PPC_BOOK3S
 	/* No MSR:RI on BookE */
 	andi.	r10,r8,MSR_RI
-	beq-	unrecov_restore
+	beq-	.Lunrecov_restore
 #endif
 	/*
 	 * Disable interrupts so current_thread_info()->flags can't change,
@@ -425,6 +425,7 @@ _GLOBAL(save_nvgprs)
 	clrrdi	r0,r11,1
 	std	r0,_TRAP(r1)
 	blr
+_ASM_NOKPROBE_SYMBOL(save_nvgprs);
 
 	
 /*
@@ -795,11 +796,11 @@ restore:
 	ld	r5,SOFTE(r1)
 	lbz	r6,PACASOFTIRQEN(r13)
 	cmpwi	cr0,r5,0
-	beq	restore_irq_off
+	beq	.Lrestore_irq_off
 
 	/* We are enabling, were we already enabled ? Yes, just return */
 	cmpwi	cr0,r6,1
-	beq	cr0,do_restore
+	beq	cr0,.Ldo_restore
 
 	/*
 	 * We are about to soft-enable interrupts (we are hard disabled
@@ -808,14 +809,14 @@ restore:
 	 */
 	lbz	r0,PACAIRQHAPPENED(r13)
 	cmpwi	cr0,r0,0
-	bne-	restore_check_irq_replay
+	bne-	.Lrestore_check_irq_replay
 
 	/*
 	 * Get here when nothing happened while soft-disabled, just
 	 * soft-enable and move-on. We will hard-enable as a side
 	 * effect of rfi
 	 */
-restore_no_replay:
+.Lrestore_no_replay:
 	TRACE_ENABLE_INTS
 	li	r0,1
 	stb	r0,PACASOFTIRQEN(r13);
@@ -823,7 +824,7 @@ restore_no_replay:
 	/*
 	 * Final return path. BookE is handled in a different file
 	 */
-do_restore:
+.Ldo_restore:
 #ifdef CONFIG_PPC_BOOK3E
 	b	exception_return_book3e
 #else
@@ -857,7 +858,7 @@ fast_exception_return:
 	REST_8GPRS(5, r1)
 
 	andi.	r0,r3,MSR_RI
-	beq-	unrecov_restore
+	beq-	.Lunrecov_restore
 
 	/* Load PPR from thread struct before we clear MSR:RI */
 BEGIN_FTR_SECTION
@@ -915,7 +916,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	 * make sure that in this case, we also clear PACA_IRQ_HARD_DIS
 	 * or that bit can get out of sync and bad things will happen
 	 */
-restore_irq_off:
+.Lrestore_irq_off:
 	ld	r3,_MSR(r1)
 	lbz	r7,PACAIRQHAPPENED(r13)
 	andi.	r0,r3,MSR_EE
@@ -925,13 +926,13 @@ restore_irq_off:
 1:	li	r0,0
 	stb	r0,PACASOFTIRQEN(r13);
 	TRACE_DISABLE_INTS
-	b	do_restore
+	b	.Ldo_restore
 
 	/*
 	 * Something did happen, check if a re-emit is needed
 	 * (this also clears paca->irq_happened)
 	 */
-restore_check_irq_replay:
+.Lrestore_check_irq_replay:
 	/* XXX: We could implement a fast path here where we check
 	 * for irq_happened being just 0x01, in which case we can
 	 * clear it and return. That means that we would potentially
@@ -941,7 +942,7 @@ restore_check_irq_replay:
 	 */
 	bl	__check_irq_replay
 	cmpwi	cr0,r3,0
- 	beq	restore_no_replay
+	beq	.Lrestore_no_replay
  
 	/*
 	 * We need to re-emit an interrupt. We do so by re-using our
@@ -990,10 +991,18 @@ restore_check_irq_replay:
 #endif /* CONFIG_PPC_DOORBELL */
 1:	b	ret_from_except /* What else to do here ? */
  
-unrecov_restore:
+.Lunrecov_restore:
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	unrecoverable_exception
-	b	unrecov_restore
+	b	.Lunrecov_restore
+
+_ASM_NOKPROBE_SYMBOL(ret_from_except);
+_ASM_NOKPROBE_SYMBOL(ret_from_except_lite);
+_ASM_NOKPROBE_SYMBOL(resume_kernel);
+_ASM_NOKPROBE_SYMBOL(fast_exc_return_irq);
+_ASM_NOKPROBE_SYMBOL(restore);
+_ASM_NOKPROBE_SYMBOL(fast_exception_return);
+
 
 #ifdef CONFIG_PPC_RTAS
 /*
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 3262e447b895..20318ceeea6a 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1591,6 +1591,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
 1:	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	kernel_bad_stack
 	b	1b
+_ASM_NOKPROBE_SYMBOL(bad_stack);
 
 /*
  * When doorbell is triggered from system reset wakeup, the message is
@@ -1652,3 +1653,4 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
 1:
 	blr
 
+_ASM_NOKPROBE_SYMBOL(__replay_interrupt)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index d4e545d27ef9..bfcfd9ef09f2 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -237,6 +237,7 @@ void die(const char *str, struct pt_regs *regs, long err)
 		err = 0;
 	oops_end(flags, regs, err);
 }
+NOKPROBE_SYMBOL(die);
 
 void user_single_step_siginfo(struct task_struct *tsk,
 				struct pt_regs *regs, siginfo_t *info)
@@ -1968,6 +1969,7 @@ void unrecoverable_exception(struct pt_regs *regs)
 	       regs->trap, regs->nip);
 	die("Unrecoverable exception", regs, SIGABRT);
 }
+NOKPROBE_SYMBOL(unrecoverable_exception);
 
 #if defined(CONFIG_BOOKE_WDT) || defined(CONFIG_40x)
 /*
@@ -1998,6 +2000,7 @@ void kernel_bad_stack(struct pt_regs *regs)
 	       regs->gpr[1], regs->nip);
 	die("Bad kernel stack pointer", regs, SIGABRT);
 }
+NOKPROBE_SYMBOL(kernel_bad_stack);
 
 void __init trap_init(void)
 {
-- 
2.13.1

[PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Naveen N. Rao]

We can't take traps with relocation off, so blacklist enter_rtas() and
rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
introduce a new symbol __enter_rtas from where on we can't take a trap
and blacklist that.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_64.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 0c27084800b6..16f4c4a1a294 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
 	sync				/* disable interrupts so SRR0/1 */
 	mtmsrd	r0			/* don't get trashed */
 
+__enter_rtas:
 	LOAD_REG_ADDR(r4, rtas)
 	ld	r5,RTASENTRY(r4)	/* get the rtas->entry value */
 	ld	r4,RTASBASE(r4)		/* get the rtas->base value */
@@ -1115,6 +1116,8 @@ rtas_return_loc:
 	mtspr	SPRN_SRR1,r4
 	rfid
 	b	.	/* prevent speculative execution */
+_ASM_NOKPROBE_SYMBOL(__enter_rtas)
+_ASM_NOKPROBE_SYMBOL(rtas_return_loc)
 
 	.align	3
 1:	.llong	rtas_restore_regs
-- 
2.13.1

Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Nicholas Piggin]

On Thu, 29 Jun 2017 16:11:10 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> We can't take traps with relocation off, so blacklist enter_rtas() and
> rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
> introduce a new symbol __enter_rtas from where on we can't take a trap
> and blacklist that.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/entry_64.S | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 0c27084800b6..16f4c4a1a294 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
>  	sync				/* disable interrupts so SRR0/1 */
>  	mtmsrd	r0			/* don't get trashed */
>  
> +__enter_rtas:

Hmm, am I missing something, or is there a reason to put these labels
after the mtmsr? Even if kprobes does the right thing, I think it's
easier to read the code if you cover the mtmsr as well.

Thanks,
Nick

Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Naveen N. Rao]

On 2017/06/29 09:01PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 16:11:10 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > We can't take traps with relocation off, so blacklist enter_rtas() and
> > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
> > introduce a new symbol __enter_rtas from where on we can't take a trap
> > and blacklist that.
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/entry_64.S | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 0c27084800b6..16f4c4a1a294 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> >  	sync				/* disable interrupts so SRR0/1 */
> >  	mtmsrd	r0			/* don't get trashed */
> >  
> > +__enter_rtas:
> 
> Hmm, am I missing something, or is there a reason to put these labels
> after the mtmsr? Even if kprobes does the right thing, I think it's
> easier to read the code if you cover the mtmsr as well.

I thought you asked for this, per your previous review comment:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html

Or, did I get that wrong?

Thanks,
Naveen

Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Nicholas Piggin]

On Thu, 29 Jun 2017 17:24:14 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/06/29 09:01PM, Nicholas Piggin wrote:
> > On Thu, 29 Jun 2017 16:11:10 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> >   
> > > We can't take traps with relocation off, so blacklist enter_rtas() and
> > > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
> > > introduce a new symbol __enter_rtas from where on we can't take a trap
> > > and blacklist that.
> > > 
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > ---
> > >  arch/powerpc/kernel/entry_64.S | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > > index 0c27084800b6..16f4c4a1a294 100644
> > > --- a/arch/powerpc/kernel/entry_64.S
> > > +++ b/arch/powerpc/kernel/entry_64.S
> > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> > >  	sync				/* disable interrupts so SRR0/1 */
> > >  	mtmsrd	r0			/* don't get trashed */
> > >  
> > > +__enter_rtas:  
> > 
> > Hmm, am I missing something, or is there a reason to put these labels
> > after the mtmsr? Even if kprobes does the right thing, I think it's
> > easier to read the code if you cover the mtmsr as well.  
> 
> I thought you asked for this, per your previous review comment:
> https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html
> 
> Or, did I get that wrong?

No you're right, I'm contradicting myself. Let me start again.

I think we'd like to put the label before the mtmsrd if possible. So
in that case, should we adjust the system call code instead (then you
wouldn't have to add a comment for it).

And then you could move this label back above the mtmsrd. Sorry for
the confusion.

Thanks,
Nick

Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Naveen N. Rao]

On 2017/06/29 10:13PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 17:24:14 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > On 2017/06/29 09:01PM, Nicholas Piggin wrote:
> > > On Thu, 29 Jun 2017 16:11:10 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > >   
> > > > We can't take traps with relocation off, so blacklist enter_rtas() and
> > > > rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
> > > > introduce a new symbol __enter_rtas from where on we can't take a trap
> > > > and blacklist that.
> > > > 
> > > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > > ---
> > > >  arch/powerpc/kernel/entry_64.S | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > > > index 0c27084800b6..16f4c4a1a294 100644
> > > > --- a/arch/powerpc/kernel/entry_64.S
> > > > +++ b/arch/powerpc/kernel/entry_64.S
> > > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> > > >  	sync				/* disable interrupts so SRR0/1 */
> > > >  	mtmsrd	r0			/* don't get trashed */
> > > >  
> > > > +__enter_rtas:  
> > > 
> > > Hmm, am I missing something, or is there a reason to put these labels
> > > after the mtmsr? Even if kprobes does the right thing, I think it's
> > > easier to read the code if you cover the mtmsr as well.  
> > 
> > I thought you asked for this, per your previous review comment:
> > https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html
> > 
> > Or, did I get that wrong?
> 
> No you're right, I'm contradicting myself. Let me start again.
> 
> I think we'd like to put the label before the mtmsrd if possible. So
> in that case, should we adjust the system call code instead (then you
> wouldn't have to add a comment for it).
> 
> And then you could move this label back above the mtmsrd. Sorry for
> the confusion.

Sure - I now get why you were insisting on a comment with the 
system_call_exit symbol. v5 enroute...

Thanks,
Naveen