| Message ID | 1771166.6Dy2cXd33p@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 1, 2016 at 5:38 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Monday, August 01, 2016 10:08:00 AM Thomas Garnier wrote: >> When KASLR memory randomization is used, __PAGE_OFFSET is a global >> variable changed during boot. The assembly code was using the variable >> as an immediate value to calculate the cr3 physical address. The >> physical address was incorrect resulting to a GP fault. >> >> Signed-off-by: Thomas Garnier <thgarnie@google.com> >> --- >> arch/x86/power/hibernate_asm_64.S | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S >> index 8eee0e9..8db4905 100644 >> --- a/arch/x86/power/hibernate_asm_64.S >> +++ b/arch/x86/power/hibernate_asm_64.S >> @@ -23,6 +23,16 @@ >> #include <asm/processor-flags.h> >> #include <asm/frame.h> >> >> +/* >> + * A global variable holds the page_offset when KASLR memory randomization >> + * is enabled. >> + */ >> +#ifdef CONFIG_RANDOMIZE_MEMORY >> +#define __PAGE_OFFSET_REF __PAGE_OFFSET >> +#else >> +#define __PAGE_OFFSET_REF $__PAGE_OFFSET >> +#endif >> + >> ENTRY(swsusp_arch_suspend) >> movq $saved_context, %rax >> movq %rsp, pt_regs_sp(%rax) >> @@ -72,7 +82,7 @@ ENTRY(restore_image) >> /* code below has been relocated to a safe page */ >> ENTRY(core_restore_code) >> /* switch to temporary page tables */ >> - movq $__PAGE_OFFSET, %rcx >> + movq __PAGE_OFFSET_REF, %rcx >> subq %rcx, %rax >> movq %rax, %cr3 >> /* flush TLB */ >> > > I'm not particularly liking the #ifdefs and they won't be really > necessary if the subtraction is carried out by the C code IMO. > > What about the patch below instead? > Yes, I think that's a good idea. I will test it and send PATCH v2. Thanks for the quick feedback. > --- > arch/x86/power/hibernate_64.c | 18 +++++++++--------- > arch/x86/power/hibernate_asm_64.S | 2 -- > 2 files changed, 9 insertions(+), 11 deletions(-) > > 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 > @@ -72,8 +72,6 @@ ENTRY(restore_image) > /* 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 > 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 > @@ -37,11 +37,11 @@ unsigned long jump_address_phys; > */ > unsigned long restore_cr3 __visible; > > -pgd_t *temp_level4_pgt __visible; > +unsigned long temp_level4_pgt __visible; > > unsigned long relocated_restore_code __visible; > > -static int set_up_temporary_text_mapping(void) > +static int set_up_temporary_text_mapping(pgd_t *pgd) > { > pmd_t *pmd; > pud_t *pud; > @@ -71,7 +71,7 @@ static int set_up_temporary_text_mapping > __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), > + set_pgd(pgd + pgd_index(restore_jump_address), > __pgd(__pa(pud) | _KERNPG_TABLE)); > > return 0; > @@ -90,15 +90,16 @@ static int set_up_temporary_mappings(voi > .kernel_mapping = true, > }; > unsigned long mstart, mend; > + pgd_t *pgd; > int result; > int i; > > - temp_level4_pgt = (pgd_t *)get_safe_page(GFP_ATOMIC); > - if (!temp_level4_pgt) > + pgd = (pgd_t *)get_safe_page(GFP_ATOMIC); > + if (!pgd) > return -ENOMEM; > > /* Prepare a temporary mapping for the kernel text */ > - result = set_up_temporary_text_mapping(); > + result = set_up_temporary_text_mapping(pgd); > if (result) > return result; > > @@ -107,13 +108,12 @@ static int set_up_temporary_mappings(voi > mstart = pfn_mapped[i].start << PAGE_SHIFT; > mend = pfn_mapped[i].end << PAGE_SHIFT; > > - result = kernel_ident_mapping_init(&info, temp_level4_pgt, > - mstart, mend); > - > + result = kernel_ident_mapping_init(&info, pgd, mstart, mend); > if (result) > return result; > } > > + temp_level4_pgt = (unsigned long)pgd - __PAGE_OFFSET; > return 0; > } > >
On Tue, Aug 2, 2016 at 4:34 PM, Thomas Garnier <thgarnie@google.com> wrote: > On Mon, Aug 1, 2016 at 5:38 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Monday, August 01, 2016 10:08:00 AM Thomas Garnier wrote: >>> When KASLR memory randomization is used, __PAGE_OFFSET is a global >>> variable changed during boot. The assembly code was using the variable >>> as an immediate value to calculate the cr3 physical address. The >>> physical address was incorrect resulting to a GP fault. >>> >>> Signed-off-by: Thomas Garnier <thgarnie@google.com> >>> --- >>> arch/x86/power/hibernate_asm_64.S | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S >>> index 8eee0e9..8db4905 100644 >>> --- a/arch/x86/power/hibernate_asm_64.S >>> +++ b/arch/x86/power/hibernate_asm_64.S >>> @@ -23,6 +23,16 @@ >>> #include <asm/processor-flags.h> >>> #include <asm/frame.h> >>> >>> +/* >>> + * A global variable holds the page_offset when KASLR memory randomization >>> + * is enabled. >>> + */ >>> +#ifdef CONFIG_RANDOMIZE_MEMORY >>> +#define __PAGE_OFFSET_REF __PAGE_OFFSET >>> +#else >>> +#define __PAGE_OFFSET_REF $__PAGE_OFFSET >>> +#endif >>> + >>> ENTRY(swsusp_arch_suspend) >>> movq $saved_context, %rax >>> movq %rsp, pt_regs_sp(%rax) >>> @@ -72,7 +82,7 @@ ENTRY(restore_image) >>> /* code below has been relocated to a safe page */ >>> ENTRY(core_restore_code) >>> /* switch to temporary page tables */ >>> - movq $__PAGE_OFFSET, %rcx >>> + movq __PAGE_OFFSET_REF, %rcx >>> subq %rcx, %rax >>> movq %rax, %cr3 >>> /* flush TLB */ >>> >> >> I'm not particularly liking the #ifdefs and they won't be really >> necessary if the subtraction is carried out by the C code IMO. >> >> What about the patch below instead? >> > > Yes, I think that's a good idea. I will test it and send PATCH v2. No need to send this patch again. Please just let me know if it works for you. :-) Thanks, Rafael
On Tue, Aug 2, 2016 at 1:47 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Tue, Aug 2, 2016 at 4:34 PM, Thomas Garnier <thgarnie@google.com> wrote: >> On Mon, Aug 1, 2016 at 5:38 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> On Monday, August 01, 2016 10:08:00 AM Thomas Garnier wrote: >>>> When KASLR memory randomization is used, __PAGE_OFFSET is a global >>>> variable changed during boot. The assembly code was using the variable >>>> as an immediate value to calculate the cr3 physical address. The >>>> physical address was incorrect resulting to a GP fault. >>>> >>>> Signed-off-by: Thomas Garnier <thgarnie@google.com> >>>> --- >>>> arch/x86/power/hibernate_asm_64.S | 12 +++++++++++- >>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S >>>> index 8eee0e9..8db4905 100644 >>>> --- a/arch/x86/power/hibernate_asm_64.S >>>> +++ b/arch/x86/power/hibernate_asm_64.S >>>> @@ -23,6 +23,16 @@ >>>> #include <asm/processor-flags.h> >>>> #include <asm/frame.h> >>>> >>>> +/* >>>> + * A global variable holds the page_offset when KASLR memory randomization >>>> + * is enabled. >>>> + */ >>>> +#ifdef CONFIG_RANDOMIZE_MEMORY >>>> +#define __PAGE_OFFSET_REF __PAGE_OFFSET >>>> +#else >>>> +#define __PAGE_OFFSET_REF $__PAGE_OFFSET >>>> +#endif >>>> + >>>> ENTRY(swsusp_arch_suspend) >>>> movq $saved_context, %rax >>>> movq %rsp, pt_regs_sp(%rax) >>>> @@ -72,7 +82,7 @@ ENTRY(restore_image) >>>> /* code below has been relocated to a safe page */ >>>> ENTRY(core_restore_code) >>>> /* switch to temporary page tables */ >>>> - movq $__PAGE_OFFSET, %rcx >>>> + movq __PAGE_OFFSET_REF, %rcx >>>> subq %rcx, %rax >>>> movq %rax, %cr3 >>>> /* flush TLB */ >>>> >>> >>> I'm not particularly liking the #ifdefs and they won't be really >>> necessary if the subtraction is carried out by the C code IMO. >>> >>> What about the patch below instead? >>> >> >> Yes, I think that's a good idea. I will test it and send PATCH v2. > > No need to send this patch again. Please just let me know if it works > for you. :-) > It worked well when I tested it and I agree that's a better approach. > Thanks, > Rafael
On Tue, Aug 2, 2016 at 10:59 PM, Thomas Garnier <thgarnie@google.com> wrote: > On Tue, Aug 2, 2016 at 1:47 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Tue, Aug 2, 2016 at 4:34 PM, Thomas Garnier <thgarnie@google.com> wrote: >>> On Mon, Aug 1, 2016 at 5:38 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>> On Monday, August 01, 2016 10:08:00 AM Thomas Garnier wrote: >>>>> When KASLR memory randomization is used, __PAGE_OFFSET is a global >>>>> variable changed during boot. The assembly code was using the variable >>>>> as an immediate value to calculate the cr3 physical address. The >>>>> physical address was incorrect resulting to a GP fault. >>>>> >>>>> Signed-off-by: Thomas Garnier <thgarnie@google.com> >>>>> --- >>>>> arch/x86/power/hibernate_asm_64.S | 12 +++++++++++- >>>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S >>>>> index 8eee0e9..8db4905 100644 >>>>> --- a/arch/x86/power/hibernate_asm_64.S >>>>> +++ b/arch/x86/power/hibernate_asm_64.S >>>>> @@ -23,6 +23,16 @@ >>>>> #include <asm/processor-flags.h> >>>>> #include <asm/frame.h> >>>>> >>>>> +/* >>>>> + * A global variable holds the page_offset when KASLR memory randomization >>>>> + * is enabled. >>>>> + */ >>>>> +#ifdef CONFIG_RANDOMIZE_MEMORY >>>>> +#define __PAGE_OFFSET_REF __PAGE_OFFSET >>>>> +#else >>>>> +#define __PAGE_OFFSET_REF $__PAGE_OFFSET >>>>> +#endif >>>>> + >>>>> ENTRY(swsusp_arch_suspend) >>>>> movq $saved_context, %rax >>>>> movq %rsp, pt_regs_sp(%rax) >>>>> @@ -72,7 +82,7 @@ ENTRY(restore_image) >>>>> /* code below has been relocated to a safe page */ >>>>> ENTRY(core_restore_code) >>>>> /* switch to temporary page tables */ >>>>> - movq $__PAGE_OFFSET, %rcx >>>>> + movq __PAGE_OFFSET_REF, %rcx >>>>> subq %rcx, %rax >>>>> movq %rax, %cr3 >>>>> /* flush TLB */ >>>>> >>>> >>>> I'm not particularly liking the #ifdefs and they won't be really >>>> necessary if the subtraction is carried out by the C code IMO. >>>> >>>> What about the patch below instead? >>>> >>> >>> Yes, I think that's a good idea. I will test it and send PATCH v2. >> >> No need to send this patch again. Please just let me know if it works >> for you. :-) >> > > It worked well when I tested it and I agree that's a better approach. OK, thanks! Let me add a changelog to it then.
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 @@ -72,8 +72,6 @@ ENTRY(restore_image) /* 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 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 @@ -37,11 +37,11 @@ unsigned long jump_address_phys; */ unsigned long restore_cr3 __visible; -pgd_t *temp_level4_pgt __visible; +unsigned long temp_level4_pgt __visible; unsigned long relocated_restore_code __visible; -static int set_up_temporary_text_mapping(void) +static int set_up_temporary_text_mapping(pgd_t *pgd) { pmd_t *pmd; pud_t *pud; @@ -71,7 +71,7 @@ static int set_up_temporary_text_mapping __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), + set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(pud) | _KERNPG_TABLE)); return 0; @@ -90,15 +90,16 @@ static int set_up_temporary_mappings(voi .kernel_mapping = true, }; unsigned long mstart, mend; + pgd_t *pgd; int result; int i; - temp_level4_pgt = (pgd_t *)get_safe_page(GFP_ATOMIC); - if (!temp_level4_pgt) + pgd = (pgd_t *)get_safe_page(GFP_ATOMIC); + if (!pgd) return -ENOMEM; /* Prepare a temporary mapping for the kernel text */ - result = set_up_temporary_text_mapping(); + result = set_up_temporary_text_mapping(pgd); if (result) return result; @@ -107,13 +108,12 @@ static int set_up_temporary_mappings(voi mstart = pfn_mapped[i].start << PAGE_SHIFT; mend = pfn_mapped[i].end << PAGE_SHIFT; - result = kernel_ident_mapping_init(&info, temp_level4_pgt, - mstart, mend); - + result = kernel_ident_mapping_init(&info, pgd, mstart, mend); if (result) return result; } + temp_level4_pgt = (unsigned long)pgd - __PAGE_OFFSET; return 0; }