Re: [PATCH v3 3/7] arm: use fixmap for text patching when text is RO

From: Kees Cook
Date: Tue Aug 12 2014 - 22:39:34 EST

On Tue, Aug 12, 2014 at 5:27 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 08/12/14 14:47, Kees Cook wrote:
>> On Tue, Aug 12, 2014 at 2:39 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
>>> On 08/12/14 11:24, Kees Cook wrote:
>>>> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
>>>> index 07314af47733..03dd4e39c833 100644
>>>> --- a/arch/arm/kernel/patch.c
>>>> +++ b/arch/arm/kernel/patch.c
>>>> @@ -13,21 +16,69 @@ 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)
>>>> +{
>>>> + 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);
>>>> +
>>>> + set_fixmap(fixmap, page_to_phys(page));
>>>> +
>>>> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
>>>> +}
>>>> +
>>>> +static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
>>>> +{
>>>> + clear_fixmap(fixmap);
>>>> +
>>>> + if (flags)
>>>> + spin_unlock_irqrestore(&patch_lock, *flags);
>>>> +}
>>> Has the kbuildbot complained about this one yet?
>>>
>>> CHECK arch/arm/kernel/patch.c
>>> arch/arm/kernel/patch.c:47:39: warning: context imbalance in
>>> 'patch_unmap' - unexpected unlock
>>>
>>> I guess we're going to ignore it.
>> No, nothing yet from buildbot, let me do a sparse run -- I think we
>> can just add annotation and we'll be okay.
>>
>>
>
> Ok. I tried to move code around but sparse still complains because of
> conditional locking.

Oh, sorry! I should have mentioned I fixed this already. It just
requires some declarations. Here's how it looks now:

https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=arm/ro-nx&id=847a4a489d31b76797e7c7a78cd3e98c7948df4b

+static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
+ __acquires(&patch_lock)
+{
...
+ if (flags)
+ spin_lock_irqsave(&patch_lock, *flags);
+ else
+ __acquire(&patch_lock);

etc.

Thanks for poking at this!

-Kees

>
> ---8<---
>
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> index 03dd4e39c833..f6d4de3826a0 100644
> --- a/arch/arm/kernel/patch.c
> +++ b/arch/arm/kernel/patch.c
> @@ -18,33 +18,17 @@ struct patch {
>
> static DEFINE_SPINLOCK(patch_lock);
>
> -static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> +static struct page __kprobes *patch_page(void *addr)
> {
> 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);
> + return vmalloc_to_page(addr);
> else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
> - page = virt_to_page(addr);
> - else
> - return addr;
> + return virt_to_page(addr);
>
> - if (flags)
> - spin_lock_irqsave(&patch_lock, *flags);
> -
> - set_fixmap(fixmap, page_to_phys(page));
> -
> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
> -}
> -
> -static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
> -{
> - clear_fixmap(fixmap);
> -
> - if (flags)
> - spin_unlock_irqrestore(&patch_lock, *flags);
> + return NULL;
> }
>
> void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
> @@ -54,10 +38,18 @@ void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
> bool twopage = false;
> unsigned long flags;
> void *waddr = addr;
> + struct page *page = NULL;
> int size;
>
> - if (remap)
> - waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
> + if (remap) {
> + page = patch_page(addr);
> + if (page) {
> + spin_lock_irqsave(&patch_lock, flags);
> + set_fixmap(FIX_TEXT_POKE0, page_to_phys(page));
> + waddr = (void *)(__fix_to_virt(FIX_TEXT_POKE0) +
> + (uintaddr & ~PAGE_MASK));
> + }
> + }
>
> if (thumb2 && __opcode_is_thumb16(insn)) {
> *(u16 *)waddr = __opcode_to_mem_thumb16(insn);
> @@ -69,15 +61,25 @@ void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
> u16 *addrh1 = waddr + 2;
>
> twopage = (uintaddr & ~PAGE_MASK) == PAGE_SIZE - 2;
> - if (twopage && remap)
> - addrh1 = patch_map(addr + 2, FIX_TEXT_POKE1, NULL);
> + if (twopage && remap) {
> + struct page *page2;
> +
> + page2 = patch_page(addr + 2);
> + if (page2) {
> + set_fixmap(FIX_TEXT_POKE1, page_to_phys(page));
> + addrh1 = (u16 *)(__fix_to_virt(FIX_TEXT_POKE1) +
> + (uintaddr & ~PAGE_MASK));
> + } else {
> + addrh1 = addr + 2;
> + }
> + }
>
> *addrh0 = __opcode_to_mem_thumb16(first);
> *addrh1 = __opcode_to_mem_thumb16(second);
>
> if (twopage && addrh1 != addr + 2) {
> flush_kernel_vmap_range(addrh1, 2);
> - patch_unmap(FIX_TEXT_POKE1, NULL);
> + clear_fixmap(FIX_TEXT_POKE1);
> }
>
> size = sizeof(u32);
> @@ -91,9 +93,10 @@ void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
> size = sizeof(u32);
> }
>
> - if (waddr != addr) {
> + if (page) {
> flush_kernel_vmap_range(waddr, twopage ? size / 2 : size);
> - patch_unmap(FIX_TEXT_POKE0, &flags);
> + clear_fixmap(FIX_TEXT_POKE0);
> + spin_unlock_irqrestore(&patch_lock, flags);
> }
>
> flush_icache_range((uintptr_t)(addr),
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>



--
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/