lkml.org 
[lkml]   [2016]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3] x86/power/64: Fix kernel text mapping corruption during image restoration
Hey,

This version is working for me as well. Thanks.

Logan

On 27/06/16 08:24 AM, Rafael J. Wysocki wrote:
> On Tuesday, June 21, 2016 11:04:41 AM Kees Cook wrote:
>> On Mon, Jun 20, 2016 at 9:35 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>> Hey Rafael,
>>>
>>> This patch appears to be working on my laptop. Thanks.
>>
>> Same for me: resume still works with KASLR in my tests too.
>
> Unfortunately, Boris still sees post-resume memory corruption with the patch
> you have tested, but that turns out to be a result of some super-weird
> corruption of a pointer on the stack which leads to a store at a wrong address
> (and there's no way it can be related to the rest of the patch).
>
> We have verified that it can be avoided by rearranging set_up_temporary_text_mapping()
> to use fewer local variables and the appended patch contains this modification.
>
> I also went on and changed relocate_restore_code() slightly in a similar fashion,
> but all of those changes should not affect the behavior (unless there's something
> insane going on behind the curtains, that is).
>
> Kees, Logan, Boris, please try this one and let me know if it works for you.
>
> Thanks,
> Rafael
>
>
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH v2] x86/power/64: Fix kernel text mapping corruption during image restoration
>
> Logan Gunthorpe reports that hibernation stopped working reliably for
> him after commit ab76f7b4ab23 (x86/mm: Set NX on gap between __ex_table
> and rodata).
>
> That turns out to be a consequence of a long-standing issue with the
> 64-bit image restoration code on x86, which is that the temporary
> page tables set up by it to avoid page tables corruption when the
> last bits of the image kernel's memory contents are copied into
> their original page frames re-use the boot kernel's text mapping,
> but that mapping may very well get corrupted just like any other
> part of the page tables. Of course, if that happens, the final
> jump to the image kernel's entry point will go to nowhere.
>
> The exact reason why commit ab76f7b4ab23 matters here is that it
> sometimes causes a PMD of a large page to be split into PTEs
> that are allocated dynamically and get corrupted during image
> restoration as described above.
>
> To fix that issue note that the code copying the last bits of the
> image kernel's memory contents to the page frames occupied by them
> previoulsy doesn't use the kernel text mapping, because it runs from
> a special page covered by the identity mapping set up for that code
> from scratch. Hence, the kernel text mapping is only needed before
> that code starts to run and then it will only be used just for the
> final jump to the image kernel's entry point.
>
> Accordingly, the temporary page tables set up in swsusp_arch_resume()
> on x86-64 need to contain the kernel text mapping too. That mapping
> is only going to be used for the final jump to the image kernel, so
> it only needs to cover the image kernel's entry point, because the
> first thing the image kernel does after getting control back is to
> switch over to its own original page tables. Moreover, the virtual
> address of the image kernel's entry point in that mapping has to be
> the same as the one mapped by the image kernel's page tables.
>
> With that in mind, modify the x86-64's arch_hibernation_header_save()
> and arch_hibernation_header_restore() routines to pass the physical
> address of the image kernel's entry point (in addition to its virtual
> address) to the boot kernel (a small piece of assembly code involved
> in passing the entry point's virtual address to the image kernel is
> not necessary any more after that, so drop it). Update RESTORE_MAGIC
> too to reflect the image header format change.
>
> Next, in set_up_temporary_mappings(), use the physical and virtual
> addresses of the image kernel's entry point passed in the image
> header to set up a minimum kernel text mapping (using memory pages
> that won't be overwritten by the image kernel's memory contents) that
> will map those addresses to each other as appropriate.
>
> This makes the concern about the possible corruption of the original
> boot kernel text mapping go away and if the the minimum kernel text
> mapping used for the final jump marks the image kernel's entry point
> memory as executable, the jump to it is guaraneed to succeed.
>
> Fixes: ab76f7b4ab23 (x86/mm: Set NX on gap between __ex_table and rodata)
> Link: http://marc.info/?l=linux-pm&m=146372852823760&w=2
> Reported-by: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> arch/x86/power/hibernate_64.c | 90 ++++++++++++++++++++++++++++++++------
> arch/x86/power/hibernate_asm_64.S | 55 ++++++++++-------------
> 2 files changed, 102 insertions(+), 43 deletions(-)
>
> Index: linux-pm/arch/x86/power/hibernate_64.c
> ===================================================================
> --- linux-pm.orig/arch/x86/power/hibernate_64.c
> +++ linux-pm/arch/x86/power/hibernate_64.c
> @@ -28,6 +28,7 @@ extern asmlinkage __visible int restore_
> * kernel's text (this value is passed in the image header).
> */
> unsigned long restore_jump_address __visible;
> +unsigned long jump_address_phys;
>
> /*
> * Value of the cr3 register from before the hibernation (this value is passed
> @@ -37,7 +38,43 @@ unsigned long restore_cr3 __visible;
>
> pgd_t *temp_level4_pgt __visible;
>
> -void *relocated_restore_code __visible;
> +unsigned long relocated_restore_code __visible;
> +
> +static int set_up_temporary_text_mapping(void)
> +{
> + pmd_t *pmd;
> + pud_t *pud;
> +
> + /*
> + * The new mapping only has to cover the page containing the image
> + * kernel's entry point (jump_address_phys), because the switch over to
> + * it is carried out by relocated code running from a page allocated
> + * specifically for this purpose and covered by the identity mapping, so
> + * the temporary kernel text mapping is only needed for the final jump.
> + * Moreover, in that mapping the virtual address of the image kernel's
> + * entry point must be the same as its virtual address in the image
> + * kernel (restore_jump_address), so the image kernel's
> + * restore_registers() code doesn't find itself in a different area of
> + * the virtual address space after switching over to the original page
> + * tables used by the image kernel.
> + */
> + pud = (pud_t *)get_safe_page(GFP_ATOMIC);
> + if (!pud)
> + return -ENOMEM;
> +
> + pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
> + if (!pmd)
> + return -ENOMEM;
> +
> + set_pmd(pmd + pmd_index(restore_jump_address),
> + __pmd((jump_address_phys & PMD_MASK) | __PAGE_KERNEL_LARGE_EXEC));
> + set_pud(pud + pud_index(restore_jump_address),
> + __pud(__pa(pmd) | _KERNPG_TABLE));
> + set_pgd(temp_level4_pgt + pgd_index(restore_jump_address),
> + __pgd(__pa(pud) | _KERNPG_TABLE));
> +
> + return 0;
> +}
>
> static void *alloc_pgt_page(void *context)
> {
> @@ -59,9 +96,10 @@ static int set_up_temporary_mappings(voi
> if (!temp_level4_pgt)
> return -ENOMEM;
>
> - /* It is safe to reuse the original kernel mapping */
> - set_pgd(temp_level4_pgt + pgd_index(__START_KERNEL_map),
> - init_level4_pgt[pgd_index(__START_KERNEL_map)]);
> + /* Prepare a temporary mapping for the kernel text */
> + result = set_up_temporary_text_mapping();
> + if (result)
> + return result;
>
> /* Set up the direct mapping from scratch */
> for (i = 0; i < nr_pfn_mapped; i++) {
> @@ -78,19 +116,44 @@ static int set_up_temporary_mappings(voi
> return 0;
> }
>
> +static int relocate_restore_code(void)
> +{
> + pgd_t *pgd;
> + pmd_t *pmd;
> +
> + relocated_restore_code = get_safe_page(GFP_ATOMIC);
> + if (!relocated_restore_code)
> + return -ENOMEM;
> +
> + memcpy((void *)relocated_restore_code, &core_restore_code, PAGE_SIZE);
> +
> + /* Make the page containing the relocated code executable */
> + pgd = (pgd_t *)__va(read_cr3()) + pgd_index(relocated_restore_code);
> + pmd = pmd_offset(pud_offset(pgd, relocated_restore_code),
> + relocated_restore_code);
> + if (pmd_large(*pmd)) {
> + set_pmd(pmd, __pmd(pmd_val(*pmd) & ~_PAGE_NX));
> + } else {
> + pte_t *pte = pte_offset_kernel(pmd, relocated_restore_code);
> +
> + set_pte(pte, __pte(pte_val(*pte) & ~_PAGE_NX));
> + }
> +
> + return 0;
> +}
> +
> int swsusp_arch_resume(void)
> {
> int error;
>
> /* We have got enough memory and from now on we cannot recover */
> - if ((error = set_up_temporary_mappings()))
> + error = set_up_temporary_mappings();
> + if (error)
> return error;
>
> - relocated_restore_code = (void *)get_safe_page(GFP_ATOMIC);
> - if (!relocated_restore_code)
> - return -ENOMEM;
> - memcpy(relocated_restore_code, &core_restore_code,
> - &restore_registers - &core_restore_code);
> + error = relocate_restore_code();
> + if (error)
> + return error;
>
> restore_image();
> return 0;
> @@ -109,11 +172,12 @@ int pfn_is_nosave(unsigned long pfn)
>
> struct restore_data_record {
> unsigned long jump_address;
> + unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> };
>
> -#define RESTORE_MAGIC 0x0123456789ABCDEFUL
> +#define RESTORE_MAGIC 0x123456789ABCDEF0UL
>
> /**
> * arch_hibernation_header_save - populate the architecture specific part
> @@ -126,7 +190,8 @@ int arch_hibernation_header_save(void *a
>
> if (max_size < sizeof(struct restore_data_record))
> return -EOVERFLOW;
> - rdr->jump_address = restore_jump_address;
> + rdr->jump_address = (unsigned long)&restore_registers;
> + rdr->jump_address_phys = __pa_symbol(&restore_registers);
> rdr->cr3 = restore_cr3;
> rdr->magic = RESTORE_MAGIC;
> return 0;
> @@ -142,6 +207,7 @@ int arch_hibernation_header_restore(void
> struct restore_data_record *rdr = addr;
>
> restore_jump_address = rdr->jump_address;
> + jump_address_phys = rdr->jump_address_phys;
> restore_cr3 = rdr->cr3;
> return (rdr->magic == RESTORE_MAGIC) ? 0 : -EINVAL;
> }
> Index: linux-pm/arch/x86/power/hibernate_asm_64.S
> ===================================================================
> --- linux-pm.orig/arch/x86/power/hibernate_asm_64.S
> +++ linux-pm/arch/x86/power/hibernate_asm_64.S
> @@ -44,9 +44,6 @@ ENTRY(swsusp_arch_suspend)
> pushfq
> popq pt_regs_flags(%rax)
>
> - /* save the address of restore_registers */
> - movq $restore_registers, %rax
> - movq %rax, restore_jump_address(%rip)
> /* save cr3 */
> movq %cr3, %rax
> movq %rax, restore_cr3(%rip)
> @@ -57,31 +54,34 @@ ENTRY(swsusp_arch_suspend)
> ENDPROC(swsusp_arch_suspend)
>
> ENTRY(restore_image)
> - /* switch to temporary page tables */
> - movq $__PAGE_OFFSET, %rdx
> - movq temp_level4_pgt(%rip), %rax
> - subq %rdx, %rax
> - movq %rax, %cr3
> - /* Flush TLB */
> - movq mmu_cr4_features(%rip), %rax
> - movq %rax, %rdx
> - andq $~(X86_CR4_PGE), %rdx
> - movq %rdx, %cr4; # turn off PGE
> - movq %cr3, %rcx; # flush TLB
> - movq %rcx, %cr3;
> - movq %rax, %cr4; # turn PGE back on
> -
> /* prepare to jump to the image kernel */
> - movq restore_jump_address(%rip), %rax
> - movq restore_cr3(%rip), %rbx
> + movq restore_jump_address(%rip), %r8
> + movq restore_cr3(%rip), %r9
> +
> + /* prepare to switch to temporary page tables */
> + movq temp_level4_pgt(%rip), %rax
> + movq mmu_cr4_features(%rip), %rbx
>
> /* prepare to copy image data to their original locations */
> movq restore_pblist(%rip), %rdx
> +
> + /* jump to relocated restore code */
> movq relocated_restore_code(%rip), %rcx
> jmpq *%rcx
>
> /* code below has been relocated to a safe page */
> ENTRY(core_restore_code)
> + /* switch to temporary page tables */
> + movq $__PAGE_OFFSET, %rcx
> + subq %rcx, %rax
> + movq %rax, %cr3
> + /* flush TLB */
> + movq %rbx, %rcx
> + andq $~(X86_CR4_PGE), %rcx
> + movq %rcx, %cr4; # turn off PGE
> + movq %cr3, %rcx; # flush TLB
> + movq %rcx, %cr3;
> + movq %rbx, %cr4; # turn PGE back on
> .Lloop:
> testq %rdx, %rdx
> jz .Ldone
> @@ -96,24 +96,17 @@ ENTRY(core_restore_code)
> /* progress to the next pbe */
> movq pbe_next(%rdx), %rdx
> jmp .Lloop
> +
> .Ldone:
> /* jump to the restore_registers address from the image header */
> - jmpq *%rax
> - /*
> - * NOTE: This assumes that the boot kernel's text mapping covers the
> - * image kernel's page containing restore_registers and the address of
> - * this page is the same as in the image kernel's text mapping (it
> - * should always be true, because the text mapping is linear, starting
> - * from 0, and is supposed to cover the entire kernel text for every
> - * kernel).
> - *
> - * code below belongs to the image kernel
> - */
> + jmpq *%r8
>
> + /* code below belongs to the image kernel */
> + .align PAGE_SIZE
> ENTRY(restore_registers)
> FRAME_BEGIN
> /* go back to the original page tables */
> - movq %rbx, %cr3
> + movq %r9, %cr3
>
> /* Flush TLB, including "global" things (vmalloc) */
> movq mmu_cr4_features(%rip), %rax
>

\
 
 \ /
  Last update: 2016-06-28 02:21    [W:0.141 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site