[PATCH] arm64/ftrace: fix inadvertent BUG() in trampoline check

[PATCH] arm64/ftrace: fix inadvertent BUG() in trampoline check

[Ard Biesheuvel]

The ftrace trampoline code (which deals with modules loaded out of
BL range of the core kernel) uses plt_entries_equal() to check whether
the per-module trampoline equals a zero buffer, to decide whether the
trampoline has already been initialized.

This triggers a BUG() in the opcode manipulation code, since we end
up checking the ADRP offset of a 0x0 opcode, which is not an ADRP
instruction.

So instead, add a helper to check whether a PLT is initialized, and
call that from the frace code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/module.h | 5 +++++
 arch/arm64/kernel/ftrace.c      | 3 +--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 905e1bb0e7bd..cd9f4e9d04d3 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -73,4 +73,9 @@ static inline bool is_forbidden_offset_for_adrp(void *place)
 struct plt_entry get_plt_entry(u64 dst, void *pc);
 bool plt_entries_equal(const struct plt_entry *a, const struct plt_entry *b);
 
+static inline bool plt_entry_is_initialized(const struct plt_entry *e)
+{
+	return e->adrp || e->add || e->br;
+}
+
 #endif /* __ASM_MODULE_H */
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 8e4431a8821f..07b298120182 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -107,8 +107,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		trampoline = get_plt_entry(addr, mod->arch.ftrace_trampoline);
 		if (!plt_entries_equal(mod->arch.ftrace_trampoline,
 				       &trampoline)) {
-			if (!plt_entries_equal(mod->arch.ftrace_trampoline,
-					       &(struct plt_entry){})) {
+			if (plt_entry_is_initialized(mod->arch.ftrace_trampoline)) {
 				pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
 				return -EINVAL;
 			}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/ftrace: fix inadvertent BUG() in trampoline check

[Mark Rutland]

On Sun, Apr 07, 2019 at 09:06:16PM +0200, Ard Biesheuvel wrote:
> The ftrace trampoline code (which deals with modules loaded out of
> BL range of the core kernel) uses plt_entries_equal() to check whether
> the per-module trampoline equals a zero buffer, to decide whether the
> trampoline has already been initialized.
> 
> This triggers a BUG() in the opcode manipulation code, since we end
> up checking the ADRP offset of a 0x0 opcode, which is not an ADRP
> instruction.
> 
> So instead, add a helper to check whether a PLT is initialized, and
> call that from the frace code.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Did some change break this recently, or have we never tested this?

I think this needs a CC stable (and a fixed tag?).

Otherwise, looks sound to me:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/module.h | 5 +++++
>  arch/arm64/kernel/ftrace.c      | 3 +--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index 905e1bb0e7bd..cd9f4e9d04d3 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -73,4 +73,9 @@ static inline bool is_forbidden_offset_for_adrp(void *place)
>  struct plt_entry get_plt_entry(u64 dst, void *pc);
>  bool plt_entries_equal(const struct plt_entry *a, const struct plt_entry *b);
>  
> +static inline bool plt_entry_is_initialized(const struct plt_entry *e)
> +{
> +	return e->adrp || e->add || e->br;
> +}
> +
>  #endif /* __ASM_MODULE_H */
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 8e4431a8821f..07b298120182 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -107,8 +107,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  		trampoline = get_plt_entry(addr, mod->arch.ftrace_trampoline);
>  		if (!plt_entries_equal(mod->arch.ftrace_trampoline,
>  				       &trampoline)) {
> -			if (!plt_entries_equal(mod->arch.ftrace_trampoline,
> -					       &(struct plt_entry){})) {
> +			if (plt_entry_is_initialized(mod->arch.ftrace_trampoline)) {
>  				pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
>  				return -EINVAL;
>  			}
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/ftrace: fix inadvertent BUG() in trampoline check

[Will Deacon]

On Mon, Apr 08, 2019 at 10:19:55AM +0100, Mark Rutland wrote:
> On Sun, Apr 07, 2019 at 09:06:16PM +0200, Ard Biesheuvel wrote:
> > The ftrace trampoline code (which deals with modules loaded out of
> > BL range of the core kernel) uses plt_entries_equal() to check whether
> > the per-module trampoline equals a zero buffer, to decide whether the
> > trampoline has already been initialized.
> > 
> > This triggers a BUG() in the opcode manipulation code, since we end
> > up checking the ADRP offset of a 0x0 opcode, which is not an ADRP
> > instruction.
> > 
> > So instead, add a helper to check whether a PLT is initialized, and
> > call that from the frace code.
> > 
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Did some change break this recently, or have we never tested this?
> 
> I think this needs a CC stable (and a fixed tag?).
> 
> Otherwise, looks sound to me:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Queued as a fix, with stable/fixes tag added plus your ack.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/ftrace: fix inadvertent BUG() in trampoline check

[Ard Biesheuvel]

On Mon, 8 Apr 2019 at 17:58, Will Deacon <will.deacon@arm.com> wrote:
>
> On Mon, Apr 08, 2019 at 10:19:55AM +0100, Mark Rutland wrote:
> > On Sun, Apr 07, 2019 at 09:06:16PM +0200, Ard Biesheuvel wrote:
> > > The ftrace trampoline code (which deals with modules loaded out of
> > > BL range of the core kernel) uses plt_entries_equal() to check whether
> > > the per-module trampoline equals a zero buffer, to decide whether the
> > > trampoline has already been initialized.
> > >
> > > This triggers a BUG() in the opcode manipulation code, since we end
> > > up checking the ADRP offset of a 0x0 opcode, which is not an ADRP
> > > instruction.
> > >
> > > So instead, add a helper to check whether a PLT is initialized, and
> > > call that from the frace code.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Did some change break this recently, or have we never tested this?
> >
> > I think this needs a CC stable (and a fixed tag?).
> >
> > Otherwise, looks sound to me:
> >
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Queued as a fix, with stable/fixes tag added plus your ack.
>

Looks good to me, thanks,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel