Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Dave Hansen]

On 04/19/2018 07:11 AM, Joerg Roedel wrote:
> On Thu, Apr 19, 2018 at 03:00:11PM +0200, Borislav Petkov wrote:
>> fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK
> Hmm, that hunk from above patch looks suspicious:
> 
> -   set_pgd(pgd + pgd_index(restore_jump_address), 
> __pgd(__pa(pud) | _KERNPG_TABLE));
> +   pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
> +   set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
> 
> The old code used __pa(pud) while the new one uses __pa(p4d).
> 
> Boris, can you change that back to __pa(pud) and test please?

That does look like the culprit.  I probably copy/pasted the p4d line
from above and missed that 'p4d' when converting it to pud.

Thanks for finding this, guys.

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Dave Hansen]

On 04/19/2018 07:11 AM, Joerg Roedel wrote:
> On Thu, Apr 19, 2018 at 03:00:11PM +0200, Borislav Petkov wrote:
>> fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK
> Hmm, that hunk from above patch looks suspicious:
> 
> -   set_pgd(pgd + pgd_index(restore_jump_address), 
> __pgd(__pa(pud) | _KERNPG_TABLE));
> +   pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
> +   set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
> 
> The old code used __pa(pud) while the new one uses __pa(p4d).
> 
> Boris, can you change that back to __pa(pud) and test please?

That does look like the culprit.  I probably copy/pasted the p4d line
from above and missed that 'p4d' when converting it to pud.

Thanks for finding this, guys.

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Borislav Petkov]

On Thu, Apr 19, 2018 at 05:11:27PM +0200, Joerg Roedel wrote:
> Cool, thanks for testing! I prepared a patch and will send it out if
> Boris also reports success. I'll also added your Tested-by :)

Boris reports success too.

Very good catch, thanks!

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Borislav Petkov]

On Thu, Apr 19, 2018 at 05:11:27PM +0200, Joerg Roedel wrote:
> Cool, thanks for testing! I prepared a patch and will send it out if
> Boris also reports success. I'll also added your Tested-by :)

Boris reports success too.

Very good catch, thanks!

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Joerg Roedel]

Hi Michal,

On Thu, Apr 19, 2018 at 04:58:32PM +0200, Michal Kubecek wrote:
> I had the same problem with v4.17-rc1 and fb43d6cb91ef and the change
> you suggested applied on top of
> 
>   (a) fb43d6cb91ef with cherry-picked e6f39e87b643
>   (b) current master (87ef12027b9b)
> 
> fixes the resume on my system.

Cool, thanks for testing! I prepared a patch and will send it out if
Boris also reports success. I'll also added your Tested-by :)


Thanks,

Joerg

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Joerg Roedel]

Hi Michal,

On Thu, Apr 19, 2018 at 04:58:32PM +0200, Michal Kubecek wrote:
> I had the same problem with v4.17-rc1 and fb43d6cb91ef and the change
> you suggested applied on top of
> 
>   (a) fb43d6cb91ef with cherry-picked e6f39e87b643
>   (b) current master (87ef12027b9b)
> 
> fixes the resume on my system.

Cool, thanks for testing! I prepared a patch and will send it out if
Boris also reports success. I'll also added your Tested-by :)


Thanks,

Joerg

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Michal Kubecek]

On Thu, Apr 19, 2018 at 04:11:34PM +0200, Joerg Roedel wrote:
> On Thu, Apr 19, 2018 at 03:00:11PM +0200, Borislav Petkov wrote:
> > fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK
> 
> Hmm, that hunk from above patch looks suspicious:
> 
> -   set_pgd(pgd + pgd_index(restore_jump_address), 
> __pgd(__pa(pud) | _KERNPG_TABLE));
> +   pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
> +   set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
> 
> The old code used __pa(pud) while the new one uses __pa(p4d).
> 
> Boris, can you change that back to __pa(pud) and test please?

I had the same problem with v4.17-rc1 and fb43d6cb91ef and the change
you suggested applied on top of

  (a) fb43d6cb91ef with cherry-picked e6f39e87b643
  (b) current master (87ef12027b9b)

fixes the resume on my system.

Michal Kubecek

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Joerg Roedel]

On Thu, Apr 19, 2018 at 03:00:11PM +0200, Borislav Petkov wrote:
> fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK

Hmm, that hunk from above patch looks suspicious:

-   set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(pud) 
| _KERNPG_TABLE));
+   pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
+   set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);

The old code used __pa(pud) while the new one uses __pa(p4d).

Boris, can you change that back to __pa(pud) and test please?


Thanks,

Joerg

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Joerg Roedel]

On Thu, Apr 19, 2018 at 03:00:11PM +0200, Borislav Petkov wrote:
> fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK

Hmm, that hunk from above patch looks suspicious:

-   set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(pud) 
| _KERNPG_TABLE));
+   pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
+   set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);

The old code used __pa(pud) while the new one uses __pa(p4d).

Boris, can you change that back to __pa(pud) and test please?


Thanks,

Joerg

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Borislav Petkov]

On Wed, Apr 18, 2018 at 02:18:57PM +0200, Borislav Petkov wrote:
> Btw, even with Jörg's fix,
> 
> fb43d6cb91ef ("x86/mm: Do not auto-massage page protections")

Some updates.

1. current linus master with Joerg's fix triple-faults on the IVB laptop
during resume from disk.

2. current linus master with Joerg's fix + those two:

C Apr 17   Dave Hansen ( :  67) [PATCH 1/2] x86, pti: fix boot 
problems from Global-bit setting
C Apr 17   Dave Hansen ( :  81) └─>[PATCH 2/2] x86, pti: fix boot 
warning from Global-bit setting

triplefaults too.

Bisection confirms again fb43d6cb91ef is the first bad:

fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK
6baf4bec02db x86/espfix: Document use of _PAGE_GLOBAL   <--- OK

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Borislav Petkov]

On Wed, Apr 18, 2018 at 02:18:57PM +0200, Borislav Petkov wrote:
> Btw, even with Jörg's fix,
> 
> fb43d6cb91ef ("x86/mm: Do not auto-massage page protections")

Some updates.

1. current linus master with Joerg's fix triple-faults on the IVB laptop
during resume from disk.

2. current linus master with Joerg's fix + those two:

C Apr 17   Dave Hansen ( :  67) [PATCH 1/2] x86, pti: fix boot 
problems from Global-bit setting
C Apr 17   Dave Hansen ( :  81) └─>[PATCH 2/2] x86, pti: fix boot 
warning from Global-bit setting

triplefaults too.

Bisection confirms again fb43d6cb91ef is the first bad:

fb43d6cb91ef x86/mm: Do not auto-massage page protections <--- NOT OK
6baf4bec02db x86/espfix: Document use of _PAGE_GLOBAL   <--- OK

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Borislav Petkov]

+ Rafael.

On Mon, Apr 16, 2018 at 08:39:37AM -0700, Dave Hansen wrote:
> On 04/16/2018 08:16 AM, Andy Lutomirski wrote:
> > On Mon, Apr 16, 2018 at 2:43 AM, Joerg Roedel  wrote:
> >> From: Joerg Roedel 
> >>
> >> The |= operator will let us end up with an invalid PTE. Use
> >> the correct &= instead.
> > D'oh!  Looks good.
> 
> Yes, agreed.  Thanks for finding that, Joerg!

Btw, even with Jörg's fix,

fb43d6cb91ef ("x86/mm: Do not auto-massage page protections")

is still broken. In my case

# CONFIG_MODIFY_LDT_SYSCALL is not set

so Jörg's patch doesn't have any effect.

I tried the patch before fb43d6cb91ef:

6baf4bec02db ("x86/espfix: Document use of _PAGE_GLOBAL")

and the machine's fine. But with fb43d6cb91ef I can't resume from disk
properly and I'm seeing is the below splat:

[5.417480] PM: Image loading progress:   0%
[5.631174] PM: Image loading progress:  10%
[5.716705] PM: Image loading progress:  20%
[5.805258] PM: Image loading progress:  30%
[5.884919] random: crng init done
[5.899245] PM: Image loading progress:  40%
[5.980752] PM: Image loading progress:  50%
[6.058269] PM: Image loading progress:  60%
[6.138994] PM: Image loading progress:  70%
[6.219384] PM: Image loading progress:  80%
[6.299277] PM: Image loading progress:  90%
[6.382403] PM: Image loading progress: 100%
[6.386971] PM: Image loading done
[6.390424] PM: Read 598112 kbytes in 1.03 seconds (580.69 MB/s)
[6.397450] PM: Image successfully loaded
[6.631431] ACPI: EC: interrupt blocked
[6.635383] Disabling non-boot CPUs ...
[6.821452] [ cut here ]
[6.826115] kernel BUG at arch/x86/mm/physaddr.c:27!
[6.831126] invalid opcode:  [#1] PREEMPT SMP
[6.835872] Modules linked in:
[6.838976] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.16.0+ #6
[6.845026] Hardware name: To be filled by O.E.M. To be filled by 
O.E.M./M5A97 EVO R2.0, BIOS 1503 01/16/2013
[6.854996] RIP: 0010:__phys_addr+0x38/0x50
[6.859220] RSP: 0018:c9017dc8 EFLAGS: 00010087
[6.864490] RAX: 7800 RBX: 880426f5b000 RCX: 0030
[6.871667] RDX: 8000 RSI: 1fe46a1b RDI: 
[6.878841] RBP: c9017e48 R08:  R09: 0002
[6.886018] R10: 5ad731e5 R11: 02de R12: 0063
[6.893221] R13:  R14: 016001e3 R15: 
[6.900396] FS:  () GS:88043dc0() 
knlGS:
[6.908544] CS:  0010 DS:  ES:  CR0: 80050033
[6.914333] CR2: 88043efff000 CR3: 02209000 CR4: 000406f0
[6.921507] Call Trace:
[6.924007]  swsusp_arch_resume+0x112/0x3c0
[6.928235]  ? hibernate_resume_nonboot_cpu_disable+0x30/0x30
[6.934024]  ? save_processor_state+0xc9/0x250
[6.938514]  ? set_debug_rodata+0x11/0x11
[6.942570]  hibernation_restore+0x8d/0x130
[6.946797]  ? hibernation_restore+0x130/0x130
[6.951286]  load_image_and_restore+0x5e/0x99
[6.955690]  software_resume+0x20f/0x2a0
[6.959661]  do_one_initcall+0x5c/0x1b0
[6.963544]  kernel_init_freeable+0x123/0x1a5
[6.967945]  ? rest_init+0xc0/0xc0
[6.971394]  kernel_init+0xa/0x100
[6.974869]  ret_from_fork+0x27/0x50
[6.978518] Code: 48 89 c2 72 28 48 b8 00 00 00 00 00 78 00 00 48 01 f8 48 
39 c2 72 14 0f b6 0d fe e6 22 01 48 89 c2 48 d3 ea 48 85 d2 75 02 f3 c3 <0f> 0b 
48 8b 05 0f 71 16 01 48 01 d0 48 81 fa ff ff ff 1f 76 e9 
[6.997454] RIP: __phys_addr+0x38/0x50 RSP: c9017dc8
[7.003159] ---[ end trace 4b9d4661c2110f43 ]---
[7.007820] note: swapper/0[1] exited with preempt_count 1
[7.013353] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b
[7.013353] 
[7.022563] Kernel Offset: disabled
[7.026098] ---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b
[7.026098]  ]---

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Borislav Petkov]

+ Rafael.

On Mon, Apr 16, 2018 at 08:39:37AM -0700, Dave Hansen wrote:
> On 04/16/2018 08:16 AM, Andy Lutomirski wrote:
> > On Mon, Apr 16, 2018 at 2:43 AM, Joerg Roedel  wrote:
> >> From: Joerg Roedel 
> >>
> >> The |= operator will let us end up with an invalid PTE. Use
> >> the correct &= instead.
> > D'oh!  Looks good.
> 
> Yes, agreed.  Thanks for finding that, Joerg!

Btw, even with Jörg's fix,

fb43d6cb91ef ("x86/mm: Do not auto-massage page protections")

is still broken. In my case

# CONFIG_MODIFY_LDT_SYSCALL is not set

so Jörg's patch doesn't have any effect.

I tried the patch before fb43d6cb91ef:

6baf4bec02db ("x86/espfix: Document use of _PAGE_GLOBAL")

and the machine's fine. But with fb43d6cb91ef I can't resume from disk
properly and I'm seeing is the below splat:

[5.417480] PM: Image loading progress:   0%
[5.631174] PM: Image loading progress:  10%
[5.716705] PM: Image loading progress:  20%
[5.805258] PM: Image loading progress:  30%
[5.884919] random: crng init done
[5.899245] PM: Image loading progress:  40%
[5.980752] PM: Image loading progress:  50%
[6.058269] PM: Image loading progress:  60%
[6.138994] PM: Image loading progress:  70%
[6.219384] PM: Image loading progress:  80%
[6.299277] PM: Image loading progress:  90%
[6.382403] PM: Image loading progress: 100%
[6.386971] PM: Image loading done
[6.390424] PM: Read 598112 kbytes in 1.03 seconds (580.69 MB/s)
[6.397450] PM: Image successfully loaded
[6.631431] ACPI: EC: interrupt blocked
[6.635383] Disabling non-boot CPUs ...
[6.821452] [ cut here ]
[6.826115] kernel BUG at arch/x86/mm/physaddr.c:27!
[6.831126] invalid opcode:  [#1] PREEMPT SMP
[6.835872] Modules linked in:
[6.838976] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.16.0+ #6
[6.845026] Hardware name: To be filled by O.E.M. To be filled by 
O.E.M./M5A97 EVO R2.0, BIOS 1503 01/16/2013
[6.854996] RIP: 0010:__phys_addr+0x38/0x50
[6.859220] RSP: 0018:c9017dc8 EFLAGS: 00010087
[6.864490] RAX: 7800 RBX: 880426f5b000 RCX: 0030
[6.871667] RDX: 8000 RSI: 1fe46a1b RDI: 
[6.878841] RBP: c9017e48 R08:  R09: 0002
[6.886018] R10: 5ad731e5 R11: 02de R12: 0063
[6.893221] R13:  R14: 016001e3 R15: 
[6.900396] FS:  () GS:88043dc0() 
knlGS:
[6.908544] CS:  0010 DS:  ES:  CR0: 80050033
[6.914333] CR2: 88043efff000 CR3: 02209000 CR4: 000406f0
[6.921507] Call Trace:
[6.924007]  swsusp_arch_resume+0x112/0x3c0
[6.928235]  ? hibernate_resume_nonboot_cpu_disable+0x30/0x30
[6.934024]  ? save_processor_state+0xc9/0x250
[6.938514]  ? set_debug_rodata+0x11/0x11
[6.942570]  hibernation_restore+0x8d/0x130
[6.946797]  ? hibernation_restore+0x130/0x130
[6.951286]  load_image_and_restore+0x5e/0x99
[6.955690]  software_resume+0x20f/0x2a0
[6.959661]  do_one_initcall+0x5c/0x1b0
[6.963544]  kernel_init_freeable+0x123/0x1a5
[6.967945]  ? rest_init+0xc0/0xc0
[6.971394]  kernel_init+0xa/0x100
[6.974869]  ret_from_fork+0x27/0x50
[6.978518] Code: 48 89 c2 72 28 48 b8 00 00 00 00 00 78 00 00 48 01 f8 48 
39 c2 72 14 0f b6 0d fe e6 22 01 48 89 c2 48 d3 ea 48 85 d2 75 02 f3 c3 <0f> 0b 
48 8b 05 0f 71 16 01 48 01 d0 48 81 fa ff ff ff 1f 76 e9 
[6.997454] RIP: __phys_addr+0x38/0x50 RSP: c9017dc8
[7.003159] ---[ end trace 4b9d4661c2110f43 ]---
[7.007820] note: swapper/0[1] exited with preempt_count 1
[7.013353] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b
[7.013353] 
[7.022563] Kernel Offset: disabled
[7.026098] ---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b
[7.026098]  ]---

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Dave Hansen]

On 04/16/2018 08:16 AM, Andy Lutomirski wrote:
> On Mon, Apr 16, 2018 at 2:43 AM, Joerg Roedel  wrote:
>> From: Joerg Roedel 
>>
>> The |= operator will let us end up with an invalid PTE. Use
>> the correct &= instead.
> D'oh!  Looks good.

Yes, agreed.  Thanks for finding that, Joerg!

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Dave Hansen]

On 04/16/2018 08:16 AM, Andy Lutomirski wrote:
> On Mon, Apr 16, 2018 at 2:43 AM, Joerg Roedel  wrote:
>> From: Joerg Roedel 
>>
>> The |= operator will let us end up with an invalid PTE. Use
>> the correct &= instead.
> D'oh!  Looks good.

Yes, agreed.  Thanks for finding that, Joerg!

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Andy Lutomirski]

On Mon, Apr 16, 2018 at 2:43 AM, Joerg Roedel  wrote:
> From: Joerg Roedel 
>
> The |= operator will let us end up with an invalid PTE. Use
> the correct &= instead.

D'oh!  Looks good.

Re: [PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Andy Lutomirski]

On Mon, Apr 16, 2018 at 2:43 AM, Joerg Roedel  wrote:
> From: Joerg Roedel 
>
> The |= operator will let us end up with an invalid PTE. Use
> the correct &= instead.

D'oh!  Looks good.

[PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Joerg Roedel]

From: Joerg Roedel 

The |= operator will let us end up with an invalid PTE. Use
the correct &= instead.

Fixes: fb43d6cb91ef ('x86/mm: Do not auto-massage page protections')
Cc: Dave Hansen 
Signed-off-by: Joerg Roedel 
---
 arch/x86/kernel/ldt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
index d41d896481b8..c9b14020f4dd 100644
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -166,7 +166,7 @@ map_ldt_struct(struct mm_struct *mm, struct ldt_struct 
*ldt, int slot)
 */
pte_prot = __pgprot(__PAGE_KERNEL_RO & ~_PAGE_GLOBAL);
/* Filter out unsuppored __PAGE_KERNEL* bits: */
-   pgprot_val(pte_prot) |= __supported_pte_mask;
+   pgprot_val(pte_prot) &= __supported_pte_mask;
pte = pfn_pte(pfn, pte_prot);
set_pte_at(mm, va, ptep, pte);
pte_unmap_unlock(ptep, ptl);
-- 
2.13.6

[PATCH] x86/ldt: Fix support_pte_mask filtering in map_ldt_struct()

[Joerg Roedel]

From: Joerg Roedel 

The |= operator will let us end up with an invalid PTE. Use
the correct &= instead.

Fixes: fb43d6cb91ef ('x86/mm: Do not auto-massage page protections')
Cc: Dave Hansen 
Signed-off-by: Joerg Roedel 
---
 arch/x86/kernel/ldt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
index d41d896481b8..c9b14020f4dd 100644
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -166,7 +166,7 @@ map_ldt_struct(struct mm_struct *mm, struct ldt_struct 
*ldt, int slot)
 */
pte_prot = __pgprot(__PAGE_KERNEL_RO & ~_PAGE_GLOBAL);
/* Filter out unsuppored __PAGE_KERNEL* bits: */
-   pgprot_val(pte_prot) |= __supported_pte_mask;
+   pgprot_val(pte_prot) &= __supported_pte_mask;
pte = pfn_pte(pfn, pte_prot);
set_pte_at(mm, va, ptep, pte);
pte_unmap_unlock(ptep, ptl);
-- 
2.13.6