Messages in this thread | | | Date | Mon, 18 May 2020 17:40:31 +0930 | From | "Andrew Jeffery" <> | Subject | Re: [PATCH] ARM: kprobes: Avoid fortify panic() when copying optprobe template |
| |
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@aj.id.au> > > --- > > 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
| |