[PATCH v3 1/2] arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()

On 7 June 2017 at 10:42, Will Deacon [off-list ref] wrote:

On Tue, Jun 06, 2017 at 05:00:21PM +0000, Ard Biesheuvel wrote:

quoted

When turning branch instructions into NOPs, we attempt to validate the
action by comparing the old value at the call site with the opcode of
a direct relative branch instruction pointing at the old target.

However, these call sites are statically initialized to call _mcount(),
and may be redirected via a PLT entry if the module is loaded far away
from the kernel text, leading to false negatives and spurious errors.

So skip the validation if CONFIG_ARM64_MODULE_PLTS is configured.

Signed-off-by: Ard Biesheuvel <redacted>
---
 arch/arm64/kernel/ftrace.c | 46 ++++++++++++++++++--
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 40ad08ac569a..4cb576374b82 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c

@@ -84,12 +84,52 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
                  unsigned long addr)
 {
      unsigned long pc = rec->ip;
-     u32 old, new;
+     long offset = (long)pc - (long)addr;
+     bool validate = true;
+     u32 old = 0, new;
+
+     if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
+         (offset < -SZ_128M || offset >= SZ_128M)) {
+             u32 replaced;
+
+             /*
+              * 'mod' is only set at module load time, but if we end up
+              * dealing with an out-of-range condition, we can assume it
+              * is due to a module being loaded far away from the kernel.
+              */
+             if (!mod) {
+                     preempt_disable();
+                     mod = __module_text_address(pc);
+                     preempt_enable();

The comment in __module_text_address says that preempt must be disabled so
that the module doesn't get freed under its feet, but if that's a
possibility here then it feels really dangerous to re-enable preemption
before we've done the patching. Shouldn't we take module_mutex or something?

Ah yes. I added a comment only in patch #2, on another instance in
ftrace_make_call(), and I thought it was redundant to duplicate it
here: ftrace_lock is held here, which will prevent the module from
being unloaded in the mean time, so disabling preemption is only
necessary to prevent an assert from firing.

Other than that, this looks fine. Thanks.

Will