[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 at linaro.org> wrote:
>
>> On 02/01/2018 04:34 AM, AKASHI Takahiro wrote:
>>> From: Masami Hiramatsu <mhiramat at 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 at kernel.org>
>>> Reported-by: AKASHI Takahiro <takahiro.akashi at 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