Re: [PATCH v4 4/8] arm: use fixmap for text patching when text is RO

[Kees Cook]

On Tue, Aug 19, 2014 at 7:29 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> Hello,
>
> On Wed, Aug 13, 2014 at 06:06:29PM +0100, Kees Cook wrote:
>> From: Rabin Vincent <rabin@xxxxxx>
>>
>> Use fixmaps for text patching when the kernel text is read-only,
>> inspired by x86.  This makes jump labels and kprobes work with the
>> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
>> CONFIG_DEBUG_RODATA options.
>>
>> Signed-off-by: Rabin Vincent <rabin@xxxxxx>
>> [kees: fixed up for merge with "arm: use generic fixmap.h"]
>> [kees: added parse acquire/release annotations to pass C=1 builds]
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> [...]
>
>> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
>> index 07314af47733..a1dce690446a 100644
>> --- a/arch/arm/kernel/patch.c
>> +++ b/arch/arm/kernel/patch.c
>> @@ -1,8 +1,11 @@
>>  #include <linux/kernel.h>
>> +#include <linux/spinlock.h>
>>  #include <linux/kprobes.h>
>> +#include <linux/mm.h>
>>  #include <linux/stop_machine.h>
>>
>>  #include <asm/cacheflush.h>
>> +#include <asm/fixmap.h>
>>  #include <asm/smp_plat.h>
>>  #include <asm/opcodes.h>
>>
>> @@ -13,21 +16,77 @@ struct patch {
>>       unsigned int insn;
>>  };
>>
>> -void __kprobes __patch_text(void *addr, unsigned int insn)
>> +static DEFINE_SPINLOCK(patch_lock);
>> +
>> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
>> +     __acquires(&patch_lock)
>> +{
>> +     unsigned int uintaddr = (uintptr_t) addr;
>> +     bool module = !core_kernel_text(uintaddr);
>> +     struct page *page;
>> +
>> +     if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
>> +             page = vmalloc_to_page(addr);
>> +     else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
>> +             page = virt_to_page(addr);
>> +     else
>> +             return addr;
>> +
>> +     if (flags)
>> +             spin_lock_irqsave(&patch_lock, *flags);
>> +     else
>> +             __acquire(&patch_lock);
>
> I don't understand the locking here. Why is it conditional, why do we need
> to disable interrupts, and are you just racing against yourself?

AIUI, the locking is here to avoid multiple users of the text poking
fixmaps. It's conditional because there are two fixmaps
(FIX_TEXT_POKE0 and FIX_TEXT_POKE1). Locking happens around 0 so
locking around 1 is not needed since it is only ever used when 0 is in
use. (__patch_text_real locks patch_lock before setting 0 when it uses
remapping, and if it also needs 1, it doesn't have to lock since the
lock is already held.)

>> +     set_fixmap(fixmap, page_to_phys(page));
>
> set_fixmap does TLB invalidation, right? I think that means it can block on
> 11MPCore and A15 w/ the TLBI erratum, so it's not safe to call this with
> interrupts disabled anyway.

Oh right. Hrm.

In an earlier version of this series set_fixmap did not perform TLB
invalidation. I wonder if this is not needed at all? (Wouldn't that be
nice...)

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/