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

[Ard Biesheuvel]

On 7 June 2017 at 10:42, Will Deacon <will.deacon at arm.com> wrote:
> On Tue, Jun 06, 2017 at 05:00:21PM +0000, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel at linaro.org>
>> ---
>>  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