Re: [PATCH] ARM: kprobes: Avoid fortify_panic() when copying optprobe template

[Andrew Jeffery]

On Mon, 18 May 2020, at 01:32, Russell King - ARM Linux admin wrote:
> On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote:
> > Setting both CONFIG_KPROBES=y and CONFIG_FORTIFY_SOURCE=y on ARM leads
> > to a panic in memcpy() when injecting a kprobe despite the fixes found
> > in commit e46daee53bb5 ("ARM: 8806/1: kprobes: Fix false positive with
> > FORTIFY_SOURCE") and commit 0ac569bf6a79 ("ARM: 8834/1: Fix: kprobes:
> > optimized kprobes illegal instruction").
> > 
> > arch/arm/include/asm/kprobes.h effectively declares
> > the target type of the optprobe_template_entry assembly label as a u32,
> > which leads memcpy()'s __builtin_object_size() call to determine that
> > the pointed-to object is of size four. In practical terms the symbol is
> > used as a handle for the optimised probe assembly template that is at
> > least 96 bytes in size. The symbol's use despite its type blows up the
> > memcpy() in ARM's arch_prepare_optimized_kprobe() with a false-positive
> > fortify_panic() when it should instead copy the optimised probe template
> > into place.
> > 
> > As mentioned, a couple of attempts have been made to address the issue
> > by casting a pointer to optprobe_template_entry before providing it to
> > memcpy(), however gccs such as Ubuntu 20.04's arm-linux-gnueabi-gcc
> > 9.3.0 (Ubuntu 9.3.0-10ubuntu1) see through these efforts.
> > 
> > Squash the false-positive by aliasing the template assembly with a new
> > symbol 'arm_optprobe_template'; declare it as a function object and
> > pass the function object as the argument to memcpy() such that
> > __builtin_object_size() cannot immediately determine the object size.
> > 
> > Fixes: e46daee53bb5 ("ARM: 8806/1: kprobes: Fix false positive with FORTIFY_SOURCE")
> > Fixes: 0ac569bf6a79 ("ARM: 8834/1: Fix: kprobes: optimized kprobes illegal instruction")
> > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > ---
> >  arch/arm/include/asm/kprobes.h    | 7 +++++++
> >  arch/arm/probes/kprobes/opt-arm.c | 4 +++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
> > index 213607a1f45c..94db8bf25f9c 100644
> > --- a/arch/arm/include/asm/kprobes.h
> > +++ b/arch/arm/include/asm/kprobes.h
> > @@ -43,6 +43,13 @@ int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
> >  int kprobe_exceptions_notify(struct notifier_block *self,
> >  			     unsigned long val, void *data);
> >  
> > +/*
> > + * The optprobe template buffer is not anything that should be called directly,
> > + * however describe it as a function to give ourselves a handle to it that
> > + * bypasses CONFIG_FORTIFY_SOURCE=y sanity checks in memcpy().
> > + */
> > +extern __visible void arm_optprobe_template(void);
> 
> Does this really need to be globally visible to anything that happens
> to include this header?
> 
> While we may abhor "extern" declarations and prototypes in .c files, it
> seems to me to be entirely reasonable for this to live in opt-arm.c and
> remove the .global for this symbol, thereby making this symbol local to
> opt-arm.c

You are right, exposing it globally was unnecessary, I got caught up poking
at the other symbols. But I think we should go with Kees' patch instead.

Thanks for the quick feedback.

Andrew