[PATCH 0/2] powerpc: kprobes: blacklist exception vectors

[PATCH 0/2] powerpc: kprobes: blacklist exception vectors

[Naveen N. Rao]

This is the first of a series of patches to build up a suitable kprobes
blacklist. This series only blacklists the exception vectors.

While I have more patches in the works to blacklist other symbols, I
wanted to get some early feedback on these two patches to ensure that
the approach is ok. So, I'm posting these right away.

I'd especially appreciate a review of the first patch and feedback on
whether it does the right thing with/without relocation. My tests
didn't reveal any issues.


Thanks,
Naveen

Naveen N. Rao (2):
  powerpc: kprobes: blacklist exception handlers
  powerpc: kprobes: blacklist exception common handlers

 arch/powerpc/include/asm/head-64.h  | 1 +
 arch/powerpc/include/asm/sections.h | 1 +
 arch/powerpc/kernel/kprobes.c       | 9 +++++++++
 arch/powerpc/kernel/vmlinux.lds.S   | 2 ++
 4 files changed, 13 insertions(+)

-- 
2.12.1

[PATCH 1/2] powerpc: kprobes: blacklist exception handlers

[Naveen N. Rao]

Introduce __head_end to mark end of the early fixed sections and use the
same to blacklist all exception handlers from kprobes.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/sections.h | 1 +
 arch/powerpc/kernel/kprobes.c       | 9 +++++++++
 arch/powerpc/kernel/vmlinux.lds.S   | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 7dc006b58369..5d0c5b103302 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -10,6 +10,7 @@
 
 extern char __start_interrupts[];
 extern char __end_interrupts[];
+extern char __head_end[];
 
 extern char __prom_init_toc_start[];
 extern char __prom_init_toc_end[];
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 71286dfd76a0..59159337a097 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -35,6 +35,7 @@
 #include <asm/code-patching.h>
 #include <asm/cacheflush.h>
 #include <asm/sstep.h>
+#include <asm/sections.h>
 #include <linux/uaccess.h>
 
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
@@ -112,6 +113,14 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 	return addr;
 }
 
+bool arch_within_kprobe_blacklist(unsigned long addr)
+{
+	return  (addr >= (unsigned long)__kprobes_text_start &&
+		 addr < (unsigned long)__kprobes_text_end) ||
+		(addr >= (unsigned long)_stext &&
+		 addr < (unsigned long)__head_end);
+}
+
 int arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 7394b770ae1f..f6eee507e0b9 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -77,6 +77,8 @@ SECTIONS
 #endif
 	} :kernel
 
+	__head_end = .;
+
 	/*
 	 * If the build dies here, it's likely code in head_64.S is referencing
 	 * labels it can't reach, and the linker inserting stubs without the
-- 
2.12.1

[PATCH 2/2] powerpc: kprobes: blacklist exception common handlers

[Naveen N. Rao]

Blacklist all the exception common/OOL handlers as the kernel stack is
not yet setup, which means we can't take a trap at this point.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/head-64.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/include/asm/head-64.h b/arch/powerpc/include/asm/head-64.h
index 5067048daad4..86eb87382031 100644
--- a/arch/powerpc/include/asm/head-64.h
+++ b/arch/powerpc/include/asm/head-64.h
@@ -213,6 +213,7 @@ end_##sname:
 	USE_TEXT_SECTION();					\
 	.balign IFETCH_ALIGN_BYTES;				\
 	.global name;						\
+	_ASM_NOKPROBE_SYMBOL(name);				\
 	DEFINE_FIXED_SYMBOL(name);				\
 name:
 
-- 
2.12.1

Re: [PATCH 1/2] powerpc: kprobes: blacklist exception handlers

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 71286dfd76a0..59159337a097 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -112,6 +113,14 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>  	return addr;
>  }
>  
> +bool arch_within_kprobe_blacklist(unsigned long addr)
> +{
> +	return  (addr >= (unsigned long)__kprobes_text_start &&
> +		 addr < (unsigned long)__kprobes_text_end) ||
> +		(addr >= (unsigned long)_stext &&
> +		 addr < (unsigned long)__head_end);
> +}

This isn't quite right when the kernel is relocated.

_stext and __head_end will be updated to point to the relocated copy of
the kernel, eg:

# grep -e _stext /proc/kallsyms 
c000000002000000 T _stext

So you probably also want something like:

  if (_stext != PAGE_OFFSET &&
      addr >= PAGE_OFFSET &&
      addr < (PAGE_OFFSET + (__head_end - _stext)))
      return true;

But that's entirely untested :)

You can test the relocatable case by enabling CONFIG_RELOCATABLE_TEST.

cheers

Re: [PATCH 1/2] powerpc: kprobes: blacklist exception handlers

[Naveen N. Rao]

Excerpts from Michael Ellerman's message of April 20, 2017 12:03:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>=20
>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes=
.c
>> index 71286dfd76a0..59159337a097 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -112,6 +113,14 @@ kprobe_opcode_t *kprobe_lookup_name(const char *nam=
e, unsigned int offset)
>>  	return addr;
>>  }
>> =20
>> +bool arch_within_kprobe_blacklist(unsigned long addr)
>> +{
>> +	return  (addr >=3D (unsigned long)__kprobes_text_start &&
>> +		 addr < (unsigned long)__kprobes_text_end) ||
>> +		(addr >=3D (unsigned long)_stext &&
>> +		 addr < (unsigned long)__head_end);
>> +}
>=20
> This isn't quite right when the kernel is relocated.
>=20
> _stext and __head_end will be updated to point to the relocated copy of
> the kernel, eg:
>=20
> # grep -e _stext /proc/kallsyms=20
> c000000002000000 T _stext
>=20
> So you probably also want something like:
>=20
>   if (_stext !=3D PAGE_OFFSET &&
>       addr >=3D PAGE_OFFSET &&
>       addr < (PAGE_OFFSET + (__head_end - _stext)))
>       return true;

Ah, so that's for ensuring we don't allow probing at the real exception=20
vectors, which get copied down from _stext. In that case, we are covered=20
by the test for kernel_text_address() in check_kprobe_address_safe(). We=20
only allow probing from _stext to _etext.

>=20
> But that's entirely untested :)
>=20
> You can test the relocatable case by enabling CONFIG_RELOCATABLE_TEST.

Done, thanks. This is working as expected (without the need for the=20
changes above).  I am not allowed to probe at the real exception vectors=20
(and PAGE_OFFSET) as well as between _stext and __head_end.

Thanks,
Naveen

=

Re: [PATCH 1/2] powerpc: kprobes: blacklist exception handlers

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> Excerpts from Michael Ellerman's message of April 20, 2017 12:03:
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> 
>>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>>> index 71286dfd76a0..59159337a097 100644
>>> --- a/arch/powerpc/kernel/kprobes.c
>>> +++ b/arch/powerpc/kernel/kprobes.c
>>> @@ -112,6 +113,14 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>>>  	return addr;
>>>  }
>>>  
>>> +bool arch_within_kprobe_blacklist(unsigned long addr)
>>> +{
>>> +	return  (addr >= (unsigned long)__kprobes_text_start &&
>>> +		 addr < (unsigned long)__kprobes_text_end) ||
>>> +		(addr >= (unsigned long)_stext &&
>>> +		 addr < (unsigned long)__head_end);
>>> +}
>> 
>> This isn't quite right when the kernel is relocated.
>> 
>> _stext and __head_end will be updated to point to the relocated copy of
>> the kernel, eg:
>> 
>> # grep -e _stext /proc/kallsyms 
>> c000000002000000 T _stext
>> 
>> So you probably also want something like:
>> 
>>   if (_stext != PAGE_OFFSET &&
>>       addr >= PAGE_OFFSET &&
>>       addr < (PAGE_OFFSET + (__head_end - _stext)))
>>       return true;
>
> Ah, so that's for ensuring we don't allow probing at the real exception 
> vectors, which get copied down from _stext. In that case, we are covered 
> by the test for kernel_text_address() in check_kprobe_address_safe(). We 
> only allow probing from _stext to _etext.

OK good. I was thinking of is_kernel_addr() which just checks it's >
PAGE_OFFSET, but of course it needs to be a text address also.

>> You can test the relocatable case by enabling CONFIG_RELOCATABLE_TEST.
>
> Done, thanks. This is working as expected (without the need for the 
> changes above).  I am not allowed to probe at the real exception vectors 
> (and PAGE_OFFSET) as well as between _stext and __head_end.

Great.

cheers

Re: [1/2] powerpc: kprobes: blacklist exception handlers

[Michael Ellerman]

On Wed, 2017-04-19 at 15:29:51 UTC, "Naveen N. Rao" wrote:
> Introduce __head_end to mark end of the early fixed sections and use the
> same to blacklist all exception handlers from kprobes.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/7aa5b018bf36f733345f8814393b48

cheers

Re: [2/2] powerpc: kprobes: blacklist exception common handlers

[Michael Ellerman]

On Wed, 2017-04-19 at 15:29:52 UTC, "Naveen N. Rao" wrote:
> Blacklist all the exception common/OOL handlers as the kernel stack is
> not yet setup, which means we can't take a trap at this point.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/9a914aa6824ac5d5fd3195ed422f31

cheers