Re: [PATCH 1/3] arm64: kprobes: Move extable address check into arch_prepare_kprobe()

[Masami Hiramatsu <mhiramat@kernel.org>]

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>