Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453327 > unrolled thread

[PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore Thomas Garnier <thgarnie@google.com> - 2016-08-01 19:10 +0200
    Re: [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-02 02:50 +0200
      Re: [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore Thomas Garnier <thgarnie@google.com> - 2016-08-02 16:50 +0200
        Re: [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore Thomas Garnier <thgarnie@google.com> - 2016-08-02 23:00 +0200
          Re: [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore "Rafael J. Wysocki" <rafael@kernel.org> - 2016-08-02 23:20 +0200
            [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-03 01:20 +0200
              Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from  assembly code Pavel Machek <pavel@denx.de> - 2016-08-05 12:40 +0200
                Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-05 16:40 +0200
                  Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code Thomas Garnier <thgarnie@google.com> - 2016-08-05 17:30 +0200
                    Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from  assembly code Pavel Machek <pavel@denx.de> - 2016-08-06 22:10 +0200
                    Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-06 23:00 +0200
        Re: [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore "Rafael J. Wysocki" <rafael@kernel.org> - 2016-08-03 15:40 +0200

#1453327 — [PATCH v1 2/2] x86/power/64: Fix __PAGE_OFFSET usage on restore

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 */
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1453522

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?

---
 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;
 }
 

[toc] | [prev] | [next] | [standalone]

#1454633

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;
>  }
>
>

[toc] | [prev] | [next] | [standalone]

#1455470

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

[toc] | [prev] | [next] | [standalone]

#1455476

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.

[toc] | [prev] | [next] | [standalone]

#1455527 — [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When CONFIG_RANDOMIZE_MEMORY is set on x86-64, __PAGE_OFFSET becomes
a variable and using it as a symbol in the image memory restoration
assembly code under core_restore_code is not correct any more.

To avoid that problem, modify set_up_temporary_mappings() to compute
the physical address of the temporary page tables and store it in
temp_level4_pgt, so that the value of that variable is ready to be
written into CR3.  Then, the assembly code doesn't have to worry
about converting that value into a physical address and things work
regardless of whether or not CONFIG_RANDOMIZE_MEMORY is set.

Reported-and-tested-by: Thomas Garnier <thgarnie@google.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

I'm going to queue this up, so if there are any objections/concerns about it,
please let me know ASAP.

Thanks,
Rafael

---
 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;
 }
 

[toc] | [prev] | [next] | [standalone]

#1457013 — Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code

On Wed 2016-08-03 01:19:26, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> When CONFIG_RANDOMIZE_MEMORY is set on x86-64, __PAGE_OFFSET becomes
> a variable and using it as a symbol in the image memory restoration
> assembly code under core_restore_code is not correct any more.

On a related note... we should really have page_offset variable in
such case, and use that -- having __FOO_BAR not being a constant is
ugly/confusing/dangerous.

> To avoid that problem, modify set_up_temporary_mappings() to compute
> the physical address of the temporary page tables and store it in
> temp_level4_pgt, so that the value of that variable is ready to be
> written into CR3.  Then, the assembly code doesn't have to worry
> about converting that value into a physical address and things work
> regardless of whether or not CONFIG_RANDOMIZE_MEMORY is set.
> 
> Reported-and-tested-by: Thomas Garnier <thgarnie@google.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

Is similar patch needed for i386?

Best regards,
								Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1457128 — Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code

On Friday, August 05, 2016 12:37:13 PM Pavel Machek wrote:
> On Wed 2016-08-03 01:19:26, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > When CONFIG_RANDOMIZE_MEMORY is set on x86-64, __PAGE_OFFSET becomes
> > a variable and using it as a symbol in the image memory restoration
> > assembly code under core_restore_code is not correct any more.
> 
> On a related note... we should really have page_offset variable in
> such case, and use that -- having __FOO_BAR not being a constant is
> ugly/confusing/dangerous.
> 
> > To avoid that problem, modify set_up_temporary_mappings() to compute
> > the physical address of the temporary page tables and store it in
> > temp_level4_pgt, so that the value of that variable is ready to be
> > written into CR3.  Then, the assembly code doesn't have to worry
> > about converting that value into a physical address and things work
> > regardless of whether or not CONFIG_RANDOMIZE_MEMORY is set.
> > 
> > Reported-and-tested-by: Thomas Garnier <thgarnie@google.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> Is similar patch needed for i386?

Yes, it is, in general, for i386 hibernation to work with ASLR.

But it doesn't work with it for other reasons ATM, AFAICS.

Unfortunately, I won't really have the time to take care of this any time
soon.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1457160 — Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code

On Fri, Aug 5, 2016 at 7:44 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, August 05, 2016 12:37:13 PM Pavel Machek wrote:
>> On Wed 2016-08-03 01:19:26, Rafael J. Wysocki wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > When CONFIG_RANDOMIZE_MEMORY is set on x86-64, __PAGE_OFFSET becomes
>> > a variable and using it as a symbol in the image memory restoration
>> > assembly code under core_restore_code is not correct any more.
>>
>> On a related note... we should really have page_offset variable in
>> such case, and use that -- having __FOO_BAR not being a constant is
>> ugly/confusing/dangerous.
>>
>> > To avoid that problem, modify set_up_temporary_mappings() to compute
>> > the physical address of the temporary page tables and store it in
>> > temp_level4_pgt, so that the value of that variable is ready to be
>> > written into CR3.  Then, the assembly code doesn't have to worry
>> > about converting that value into a physical address and things work
>> > regardless of whether or not CONFIG_RANDOMIZE_MEMORY is set.
>> >
>> > Reported-and-tested-by: Thomas Garnier <thgarnie@google.com>
>> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>
>> Is similar patch needed for i386?
>
> Yes, it is, in general, for i386 hibernation to work with ASLR.
>
> But it doesn't work with it for other reasons ATM, AFAICS.
>
> Unfortunately, I won't really have the time to take care of this any time
> soon.
>

KASLR memory randomization is only available for x64 right now. I plan
on porting to 32bit eventually and will test/adapt hibernation as part
of it.

> Thanks,
> Rafael
>

[toc] | [prev] | [next] | [standalone]

#1457310 — Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code

Hi!

> > >> Is similar patch needed for i386?
> > >
> > > Yes, it is, in general, for i386 hibernation to work with ASLR.
> > >
> > > But it doesn't work with it for other reasons ATM, AFAICS.
> > >
> > > Unfortunately, I won't really have the time to take care of this any time
> > > soon.
> > >
> > 
> > KASLR memory randomization is only available for x64 right now. I plan
> > on porting to 32bit eventually and will test/adapt hibernation as part
> > of it.
> 
> Great to hear that, but you need to be aware that the i386 hibernate code has
> not been touched for a long time and it makes some heavy assumptions that
> are not made on x86-64.

Yes, we did pretty bad job keeping i386 and x86-64 in sync.

This should bring them closer together. (My original motivation was to
enable hibernation and resume using differnet kernel versions. That
worked. Merge with v4.7 changes was not trivial, but it still appears
to work, probably doing some stuff that is not neccessary on 32-bit.)

Signed-off-by: Pavel Machek <pavel@ucw.cz> (but cleanup before
applying :-))

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3a9add5..b5c48f1 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2236,7 +2236,7 @@ menu "Power management and ACPI options"
 
 config ARCH_HIBERNATION_HEADER
 	def_bool y
-	depends on X86_64 && HIBERNATION
+	depends on HIBERNATION
 
 source "kernel/power/Kconfig"
 
diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h
index 8e9dbe7..81c5bfc 100644
--- a/arch/x86/include/asm/suspend_32.h
+++ b/arch/x86/include/asm/suspend_32.h
@@ -25,4 +25,7 @@ struct saved_context {
 	unsigned long return_address;
 } __attribute__((packed));
 
+extern char core_restore_code;
+extern char restore_registers;
+
 #endif /* _ASM_X86_SUSPEND_32_H */
diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
new file mode 100644
index 0000000..c0b0572
--- /dev/null
+++ b/arch/x86/power/hibernate.c
@@ -0,0 +1,49 @@
+int reallocate_restore_code(void)
+{
+	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);
+	return 0;
+}
+
+struct restore_data_record {
+	unsigned long jump_address;
+	unsigned long jump_address_phys;
+	unsigned long cr3;
+	unsigned long magic;
+};
+
+/**
+ *	arch_hibernation_header_save - populate the architecture specific part
+ *		of a hibernation image header
+ *	@addr: address to save the data at
+ */
+int arch_hibernation_header_save(void *addr, unsigned int max_size)
+{
+	struct restore_data_record *rdr = addr;
+
+	if (max_size < sizeof(struct restore_data_record))
+		return -EOVERFLOW;
+	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;
+}
+
+/**
+ *	arch_hibernation_header_restore - read the architecture specific data
+ *		from the hibernation image header
+ *	@addr: address to read the data from
+ */
+int arch_hibernation_header_restore(void *addr)
+{
+	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;
+}
diff --git a/arch/x86/power/hibernate_32.c b/arch/x86/power/hibernate_32.c
index 9f14bd3..784e6c7 100644
--- a/arch/x86/power/hibernate_32.c
+++ b/arch/x86/power/hibernate_32.c
@@ -4,6 +4,7 @@
  * Distribute under GPLv2
  *
  * Copyright (c) 2006 Rafael J. Wysocki <rjw@sisk.pl>
+ * Copyright (c) 2015 Pavel Machek <pavel@ucw.cz>
  */
 
 #include <linux/gfp.h>
@@ -14,13 +15,30 @@
 #include <asm/pgtable.h>
 #include <asm/mmzone.h>
 #include <asm/sections.h>
+#include <asm/suspend.h>
+#include <asm/tlbflush.h>
 
 /* Defined in hibernate_asm_32.S */
 extern int restore_image(void);
 
+/*
+ * Address to jump to in the last phase of restore in order to get to the image
+ * 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
+ * in the image header).
+ */
+unsigned long restore_cr3 __visible;
+
 /* Pointer to the temporary resume page tables */
 pgd_t *resume_pg_dir;
 
+void *relocated_restore_code __visible;
+
 /* The following three functions are based on the analogous code in
  * arch/x86/mm/init_32.c
  */
@@ -142,6 +160,9 @@ static inline void resume_init_first_level_page_table(pgd_t *pg_dir)
 #endif
 }
 
+#define RESTORE_MAGIC	0x1bea1e0UL
+#include "hibernate.c"
+
 int swsusp_arch_resume(void)
 {
 	int error;
@@ -155,6 +176,10 @@ int swsusp_arch_resume(void)
 	if (error)
 		return error;
 
+	error = reallocate_restore_code();
+	if (error)
+		return error;
+
 	/* We have got enough memory and from now on we cannot recover */
 	restore_image();
 	return 0;
diff --git a/arch/x86/power/hibernate_64.c b/arch/x86/power/hibernate_64.c
index f2b5e6a..8aea0a1 100644
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -117,6 +117,9 @@ static int set_up_temporary_mappings(void)
 	return 0;
 }
 
+#define RESTORE_MAGIC	0x123456789ABCDEF0UL
+#include "hibernate.c"
+
 static int relocate_restore_code(void)
 {
 	pgd_t *pgd;
@@ -177,44 +180,3 @@ int pfn_is_nosave(unsigned long pfn)
 	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
 }
 
-struct restore_data_record {
-	unsigned long jump_address;
-	unsigned long jump_address_phys;
-	unsigned long cr3;
-	unsigned long magic;
-};
-
-#define RESTORE_MAGIC	0x123456789ABCDEF0UL
-
-/**
- *	arch_hibernation_header_save - populate the architecture specific part
- *		of a hibernation image header
- *	@addr: address to save the data at
- */
-int arch_hibernation_header_save(void *addr, unsigned int max_size)
-{
-	struct restore_data_record *rdr = addr;
-
-	if (max_size < sizeof(struct restore_data_record))
-		return -EOVERFLOW;
-	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;
-}
-
-/**
- *	arch_hibernation_header_restore - read the architecture specific data
- *		from the hibernation image header
- *	@addr: address to read the data from
- */
-int arch_hibernation_header_restore(void *addr)
-{
-	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;
-}
diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
index 1d0fa0e..f7e62d3 100644
--- a/arch/x86/power/hibernate_asm_32.S
+++ b/arch/x86/power/hibernate_asm_32.S
@@ -1,5 +1,14 @@
 /*
- * This may not use any stack, nor any variable that is not "NoSave":
+ * Hibernation support for i386
+ *
+ * Distribute under GPLv2.
+ *
+ * Copyright 2007 Rafael J. Wysocki <rjw@sisk.pl>
+ * Copyright 2005 Andi Kleen <ak@suse.de>
+ * Copyright 2004, 2015 Pavel Machek <pavel@ucw.cz>
+ *
+ * swsusp_arch_resume must not use any stack or any nonlocal variables while
+ * copying pages:
  *
  * Its rewriting one kernel image with another. What is stack in "old"
  * image could very well be data page in "new" image, and overwriting
@@ -23,6 +32,10 @@ ENTRY(swsusp_arch_suspend)
 	pushfl
 	popl saved_context_eflags
 
+	/* save cr3 */
+	movl	%cr3, %eax
+	movl	%eax, restore_cr3
+
 	call swsusp_save
 	ret
 
@@ -38,9 +51,27 @@ ENTRY(restore_image)
 	movl	%cr3, %eax;  # flush TLB
 	movl	%eax, %cr3
 1:
+
+	/* prepare to jump to the image kernel */
+	movl	restore_jump_address, %eax
+	movl	restore_cr3, %ebx
+
+#if 0
+	FIXME
+	/* prepare to switch to temporary page tables */
+	movq    temp_level4_pgt(%rip), %rax
+	movq    mmu_cr4_features(%rip), %rbx
+#endif
+	
+	/* prepare to copy image data to their original locations */
 	movl	restore_pblist, %edx
+
+	/* jump to relocated restore code */
+	movl	relocated_restore_code, %ecx
+	jmpl	*%ecx
 	.p2align 4,,7
 
+ENTRY(core_restore_code)
 copy_loop:
 	testl	%edx, %edx
 	jz	done
@@ -48,7 +79,7 @@ copy_loop:
 	movl	pbe_address(%edx), %esi
 	movl	pbe_orig_address(%edx), %edi
 
-	movl	$1024, %ecx
+	movl	$(PAGE_SIZE >> 2), %ecx
 	rep
 	movsl
 
@@ -57,6 +88,20 @@ copy_loop:
 	.p2align 4,,7
 
 done:
+	/* jump to the restore_registers address from the image header */
+	jmpl	*%eax
+	/*
+	 * 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
+	 */
+
+ENTRY(restore_registers)
 	/* go back to the original page tables */
 	movl	$swapper_pg_dir, %eax
 	subl	$__PAGE_OFFSET, %eax
@@ -81,4 +126,7 @@ done:
 
 	xorl	%eax, %eax
 
+	/* tell the hibernation core that we've just restored the memory */
+	movl	%eax, in_suspend
+
 	ret
diff --git a/tools/testing/selftests/power/sleep b/tools/testing/selftests/power/sleep
new file mode 100755
index 0000000..277d59d
--- /dev/null
+++ b/tools/testing/selftests/power/sleep
@@ -0,0 +1,5 @@
+#!/bin/bash
+echo 0 > /sys/class/rtc/rtc0/wakealarm
+echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
+echo mem > /sys/power/state
+




-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1457355 — Re: [PATCH] x86/power/64: Do not refer to __PAGE_OFFSET from assembly code

On Friday, August 05, 2016 08:21:31 AM Thomas Garnier wrote:
> On Fri, Aug 5, 2016 at 7:44 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, August 05, 2016 12:37:13 PM Pavel Machek wrote:
> >> On Wed 2016-08-03 01:19:26, Rafael J. Wysocki wrote:
> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >
> >> > When CONFIG_RANDOMIZE_MEMORY is set on x86-64, __PAGE_OFFSET becomes
> >> > a variable and using it as a symbol in the image memory restoration
> >> > assembly code under core_restore_code is not correct any more.
> >>
> >> On a related note... we should really have page_offset variable in
> >> such case, and use that -- having __FOO_BAR not being a constant is
> >> ugly/confusing/dangerous.
> >>
> >> > To avoid that problem, modify set_up_temporary_mappings() to compute
> >> > the physical address of the temporary page tables and store it in
> >> > temp_level4_pgt, so that the value of that variable is ready to be
> >> > written into CR3.  Then, the assembly code doesn't have to worry
> >> > about converting that value into a physical address and things work
> >> > regardless of whether or not CONFIG_RANDOMIZE_MEMORY is set.
> >> >
> >> > Reported-and-tested-by: Thomas Garnier <thgarnie@google.com>
> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Acked-by: Pavel Machek <pavel@ucw.cz>
> >>
> >> Is similar patch needed for i386?
> >
> > Yes, it is, in general, for i386 hibernation to work with ASLR.
> >
> > But it doesn't work with it for other reasons ATM, AFAICS.
> >
> > Unfortunately, I won't really have the time to take care of this any time
> > soon.
> >
> 
> KASLR memory randomization is only available for x64 right now. I plan
> on porting to 32bit eventually and will test/adapt hibernation as part
> of it.

Great to hear that, but you need to be aware that the i386 hibernate code has
not been touched for a long time and it makes some heavy assumptions that
are not made on x86-64.

Please keep me and Pavel in the loop, though.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1455799

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web