| Message ID | 20200517153959.293224-1-andrew@aj.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: kprobes: Avoid fortify_panic() when copying optprobe template | expand |
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 > + > /* optinsn template addresses */ > extern __visible kprobe_opcode_t optprobe_template_entry; > extern __visible kprobe_opcode_t optprobe_template_val; > diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c > index 7a449df0b359..59133d59616a 100644 > --- a/arch/arm/probes/kprobes/opt-arm.c > +++ b/arch/arm/probes/kprobes/opt-arm.c > @@ -31,6 +31,8 @@ > * to the stack cost of the instruction. > */ > asm ( > + ".global arm_optprobe_template\n" > + "arm_optprobe_template:\n" > ".global optprobe_template_entry\n" > "optprobe_template_entry:\n" > ".global optprobe_template_sub_sp\n" > @@ -234,7 +236,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or > } > > /* Copy arch-dep-instance from template. */ > - memcpy(code, (unsigned long *)&optprobe_template_entry, > + memcpy(code, arm_optprobe_template, > TMPL_END_IDX * sizeof(kprobe_opcode_t)); > > /* Adjust buffer according to instruction. */ > -- > 2.25.1 > >
On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote: > 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. Ah, dang. :P How about converting them all to unsized arrays, which would also allow the code to drop the "&" everywhere, I think. This is untested: diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h index 213607a1f45c..e26a278d301a 100644 --- a/arch/arm/include/asm/kprobes.h +++ b/arch/arm/include/asm/kprobes.h @@ -44,20 +44,20 @@ int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); /* optinsn template addresses */ -extern __visible kprobe_opcode_t optprobe_template_entry; -extern __visible kprobe_opcode_t optprobe_template_val; -extern __visible kprobe_opcode_t optprobe_template_call; -extern __visible kprobe_opcode_t optprobe_template_end; -extern __visible kprobe_opcode_t optprobe_template_sub_sp; -extern __visible kprobe_opcode_t optprobe_template_add_sp; -extern __visible kprobe_opcode_t optprobe_template_restore_begin; -extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn; -extern __visible kprobe_opcode_t optprobe_template_restore_end; +extern __visible kprobe_opcode_t optprobe_template_entry[]; +extern __visible kprobe_opcode_t optprobe_template_val[]; +extern __visible kprobe_opcode_t optprobe_template_call[]; +extern __visible kprobe_opcode_t optprobe_template_end[]; +extern __visible kprobe_opcode_t optprobe_template_sub_sp[]; +extern __visible kprobe_opcode_t optprobe_template_add_sp[]; +extern __visible kprobe_opcode_t optprobe_template_restore_begin[]; +extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn[]; +extern __visible kprobe_opcode_t optprobe_template_restore_end[]; #define MAX_OPTIMIZED_LENGTH 4 #define MAX_OPTINSN_SIZE \ - ((unsigned long)&optprobe_template_end - \ - (unsigned long)&optprobe_template_entry) + ((unsigned long)optprobe_template_end - \ + (unsigned long)optprobe_template_entry) #define RELATIVEJUMP_SIZE 4 struct arch_optimized_insn { diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c index 7a449df0b359..c78180172120 100644 --- a/arch/arm/probes/kprobes/opt-arm.c +++ b/arch/arm/probes/kprobes/opt-arm.c @@ -85,21 +85,21 @@ asm ( "optprobe_template_end:\n"); #define TMPL_VAL_IDX \ - ((unsigned long *)&optprobe_template_val - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_val - (unsigned long *)optprobe_template_entry) #define TMPL_CALL_IDX \ - ((unsigned long *)&optprobe_template_call - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_call - (unsigned long *)optprobe_template_entry) #define TMPL_END_IDX \ - ((unsigned long *)&optprobe_template_end - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_end - (unsigned long *)optprobe_template_entry) #define TMPL_ADD_SP \ - ((unsigned long *)&optprobe_template_add_sp - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_add_sp - (unsigned long *)optprobe_template_entry) #define TMPL_SUB_SP \ - ((unsigned long *)&optprobe_template_sub_sp - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_sub_sp - (unsigned long *)optprobe_template_entry) #define TMPL_RESTORE_BEGIN \ - ((unsigned long *)&optprobe_template_restore_begin - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_restore_begin - (unsigned long *)optprobe_template_entry) #define TMPL_RESTORE_ORIGN_INSN \ - ((unsigned long *)&optprobe_template_restore_orig_insn - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_restore_orig_insn - (unsigned long *)optprobe_template_entry) #define TMPL_RESTORE_END \ - ((unsigned long *)&optprobe_template_restore_end - (unsigned long *)&optprobe_template_entry) + ((unsigned long *)optprobe_template_restore_end - (unsigned long *)optprobe_template_entry) /* * ARM can always optimize an instruction when using ARM ISA, except @@ -234,7 +234,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or } /* Copy arch-dep-instance from template. */ - memcpy(code, (unsigned long *)&optprobe_template_entry, + memcpy(code, (unsigned long *)optprobe_template_entry, TMPL_END_IDX * sizeof(kprobe_opcode_t)); /* Adjust buffer according to instruction. */
On Mon, 18 May 2020, at 07:18, Kees Cook wrote: > On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote: > > 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. > > Ah, dang. :P > > How about converting them all to unsized arrays, which would also allow > the code to drop the "&" everywhere, I think. This is untested: Looks better than my hack. Took it for a spin under qemu and it works for me. Reviewed-by: Andrew Jeffery <andrew@aj.id.au> Tested-by: Andrew Jeffery <andrew@aj.id.au> Thanks Kees! Andrew
On Sun, 17 May 2020 14:48:52 -0700 Kees Cook <keescook@chromium.org> wrote: > On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote: > > 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. > > Ah, dang. :P > > How about converting them all to unsized arrays, which would also allow > the code to drop the "&" everywhere, I think. This is untested: > This looks good to me since it uses same technique in sections.h. Acked-by: Masami Hiramatsu <mhiramat@kernel.org> Thank you! > > diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h > index 213607a1f45c..e26a278d301a 100644 > --- a/arch/arm/include/asm/kprobes.h > +++ b/arch/arm/include/asm/kprobes.h > @@ -44,20 +44,20 @@ int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > > /* optinsn template addresses */ > -extern __visible kprobe_opcode_t optprobe_template_entry; > -extern __visible kprobe_opcode_t optprobe_template_val; > -extern __visible kprobe_opcode_t optprobe_template_call; > -extern __visible kprobe_opcode_t optprobe_template_end; > -extern __visible kprobe_opcode_t optprobe_template_sub_sp; > -extern __visible kprobe_opcode_t optprobe_template_add_sp; > -extern __visible kprobe_opcode_t optprobe_template_restore_begin; > -extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn; > -extern __visible kprobe_opcode_t optprobe_template_restore_end; > +extern __visible kprobe_opcode_t optprobe_template_entry[]; > +extern __visible kprobe_opcode_t optprobe_template_val[]; > +extern __visible kprobe_opcode_t optprobe_template_call[]; > +extern __visible kprobe_opcode_t optprobe_template_end[]; > +extern __visible kprobe_opcode_t optprobe_template_sub_sp[]; > +extern __visible kprobe_opcode_t optprobe_template_add_sp[]; > +extern __visible kprobe_opcode_t optprobe_template_restore_begin[]; > +extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn[]; > +extern __visible kprobe_opcode_t optprobe_template_restore_end[]; > > #define MAX_OPTIMIZED_LENGTH 4 > #define MAX_OPTINSN_SIZE \ > - ((unsigned long)&optprobe_template_end - \ > - (unsigned long)&optprobe_template_entry) > + ((unsigned long)optprobe_template_end - \ > + (unsigned long)optprobe_template_entry) > #define RELATIVEJUMP_SIZE 4 > > struct arch_optimized_insn { > diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c > index 7a449df0b359..c78180172120 100644 > --- a/arch/arm/probes/kprobes/opt-arm.c > +++ b/arch/arm/probes/kprobes/opt-arm.c > @@ -85,21 +85,21 @@ asm ( > "optprobe_template_end:\n"); > > #define TMPL_VAL_IDX \ > - ((unsigned long *)&optprobe_template_val - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_val - (unsigned long *)optprobe_template_entry) > #define TMPL_CALL_IDX \ > - ((unsigned long *)&optprobe_template_call - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_call - (unsigned long *)optprobe_template_entry) > #define TMPL_END_IDX \ > - ((unsigned long *)&optprobe_template_end - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_end - (unsigned long *)optprobe_template_entry) > #define TMPL_ADD_SP \ > - ((unsigned long *)&optprobe_template_add_sp - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_add_sp - (unsigned long *)optprobe_template_entry) > #define TMPL_SUB_SP \ > - ((unsigned long *)&optprobe_template_sub_sp - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_sub_sp - (unsigned long *)optprobe_template_entry) > #define TMPL_RESTORE_BEGIN \ > - ((unsigned long *)&optprobe_template_restore_begin - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_restore_begin - (unsigned long *)optprobe_template_entry) > #define TMPL_RESTORE_ORIGN_INSN \ > - ((unsigned long *)&optprobe_template_restore_orig_insn - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_restore_orig_insn - (unsigned long *)optprobe_template_entry) > #define TMPL_RESTORE_END \ > - ((unsigned long *)&optprobe_template_restore_end - (unsigned long *)&optprobe_template_entry) > + ((unsigned long *)optprobe_template_restore_end - (unsigned long *)optprobe_template_entry) > > /* > * ARM can always optimize an instruction when using ARM ISA, except > @@ -234,7 +234,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or > } > > /* Copy arch-dep-instance from template. */ > - memcpy(code, (unsigned long *)&optprobe_template_entry, > + memcpy(code, (unsigned long *)optprobe_template_entry, > TMPL_END_IDX * sizeof(kprobe_opcode_t)); > > /* Adjust buffer according to instruction. */ > > -- > Kees Cook
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
On Mon, 18 May 2020, at 11:50, Masami Hiramatsu wrote: > On Sun, 17 May 2020 14:48:52 -0700 > Kees Cook <keescook@chromium.org> wrote: > > > On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote: > > > 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. > > > > Ah, dang. :P > > > > How about converting them all to unsized arrays, which would also allow > > the code to drop the "&" everywhere, I think. This is untested: > > > > This looks good to me since it uses same technique in sections.h. > > Acked-by: Masami Hiramatsu <mhiramat@kernel.org> > Kees, Were you planning to send resend this, or were you looking for me to polish it up? Andrew
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); + /* optinsn template addresses */ extern __visible kprobe_opcode_t optprobe_template_entry; extern __visible kprobe_opcode_t optprobe_template_val; diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c index 7a449df0b359..59133d59616a 100644 --- a/arch/arm/probes/kprobes/opt-arm.c +++ b/arch/arm/probes/kprobes/opt-arm.c @@ -31,6 +31,8 @@ * to the stack cost of the instruction. */ asm ( + ".global arm_optprobe_template\n" + "arm_optprobe_template:\n" ".global optprobe_template_entry\n" "optprobe_template_entry:\n" ".global optprobe_template_sub_sp\n" @@ -234,7 +236,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or } /* Copy arch-dep-instance from template. */ - memcpy(code, (unsigned long *)&optprobe_template_entry, + memcpy(code, arm_optprobe_template, TMPL_END_IDX * sizeof(kprobe_opcode_t)); /* Adjust buffer according to instruction. */
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(-)