[PATCH] x86/boot/64: Fix crash if kernel images crosses page table boundary

[PATCH] x86/boot/64: Fix crash if kernel images crosses page table boundary

[Kirill A. Shutemov]

Kernel that boots in 5-level paging mode crashes in small percentage of
cases if KASLR is enabled.

This issue was tracked down to the case when the kernel image unpack in
the way it crosses 1G boundary. The crash was due to overrun of PMD page
table in __startup_64() and corruption of P4D page table allocated next
to it.

The issue is not visible with 4-level paging as we don't use the P4D
page table.

The root cause is that the handling of page table wraparound was lost
when part of startup_64() was rewritten in C.

Restore the handling. It also fixes the issue when the kernel image put
across 512G and, for 5-level paging, 64T boundary.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: c88d71508e36 ("x86/boot/64: Rewrite startup_64() in C")
---
 arch/x86/kernel/head64.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 4d98ba8063d1..b502c28801fc 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -190,18 +190,18 @@ unsigned long __head __startup_64(unsigned long physaddr,
 		pgd[i + 0] = (pgdval_t)p4d + pgtable_flags;
 		pgd[i + 1] = (pgdval_t)p4d + pgtable_flags;
 
-		i = (physaddr >> P4D_SHIFT) % PTRS_PER_P4D;
-		p4d[i + 0] = (pgdval_t)pud + pgtable_flags;
-		p4d[i + 1] = (pgdval_t)pud + pgtable_flags;
+		i = physaddr >> P4D_SHIFT;
+		p4d[(i + 0) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
+		p4d[(i + 1) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
 	} else {
 		i = (physaddr >> PGDIR_SHIFT) % PTRS_PER_PGD;
 		pgd[i + 0] = (pgdval_t)pud + pgtable_flags;
 		pgd[i + 1] = (pgdval_t)pud + pgtable_flags;
 	}
 
-	i = (physaddr >> PUD_SHIFT) % PTRS_PER_PUD;
-	pud[i + 0] = (pudval_t)pmd + pgtable_flags;
-	pud[i + 1] = (pudval_t)pmd + pgtable_flags;
+	i = physaddr >> PUD_SHIFT;
+	pud[(i + 0) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
+	pud[(i + 1) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
 
 	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
 	/* Filter out unsupported __PAGE_KERNEL_* bits: */
@@ -211,8 +211,8 @@ unsigned long __head __startup_64(unsigned long physaddr,
 	pmd_entry +=  physaddr;
 
 	for (i = 0; i < DIV_ROUND_UP(_end - _text, PMD_SIZE); i++) {
-		int idx = i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;
-		pmd[idx] = pmd_entry + i * PMD_SIZE;
+		int idx = i + (physaddr >> PMD_SHIFT);;
+		pmd[idx % PTRS_PER_PMD] = pmd_entry + i * PMD_SIZE;
 	}
 
 	/*
-- 
2.21.0

Re: [PATCH] x86/boot/64: Fix crash if kernel images crosses page table boundary

[Thomas Gleixner]

On Thu, 20 Jun 2019, Kirill A. Shutemov wrote:
> @@ -190,18 +190,18 @@ unsigned long __head __startup_64(unsigned long physaddr,
>  		pgd[i + 0] = (pgdval_t)p4d + pgtable_flags;
>  		pgd[i + 1] = (pgdval_t)p4d + pgtable_flags;
>  
> -		i = (physaddr >> P4D_SHIFT) % PTRS_PER_P4D;
> -		p4d[i + 0] = (pgdval_t)pud + pgtable_flags;
> -		p4d[i + 1] = (pgdval_t)pud + pgtable_flags;
> +		i = physaddr >> P4D_SHIFT;
> +		p4d[(i + 0) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
> +		p4d[(i + 1) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
>  	} else {
>  		i = (physaddr >> PGDIR_SHIFT) % PTRS_PER_PGD;
>  		pgd[i + 0] = (pgdval_t)pud + pgtable_flags;
>  		pgd[i + 1] = (pgdval_t)pud + pgtable_flags;
>  	}
>  
> -	i = (physaddr >> PUD_SHIFT) % PTRS_PER_PUD;
> -	pud[i + 0] = (pudval_t)pmd + pgtable_flags;
> -	pud[i + 1] = (pudval_t)pmd + pgtable_flags;
> +	i = physaddr >> PUD_SHIFT;
> +	pud[(i + 0) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
> +	pud[(i + 1) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
>  
>  	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
>  	/* Filter out unsupported __PAGE_KERNEL_* bits: */
> @@ -211,8 +211,8 @@ unsigned long __head __startup_64(unsigned long physaddr,
>  	pmd_entry +=  physaddr;
>  
>  	for (i = 0; i < DIV_ROUND_UP(_end - _text, PMD_SIZE); i++) {
> -		int idx = i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;
> -		pmd[idx] = pmd_entry + i * PMD_SIZE;
> +		int idx = i + (physaddr >> PMD_SHIFT);;

double semicolon

> +		pmd[idx % PTRS_PER_PMD] = pmd_entry + i * PMD_SIZE;

This part is functionally equivivalent. So what's the value of this change?

Thanks,

	tglx

Re: [PATCH] x86/boot/64: Fix crash if kernel images crosses page table boundary

[Kirill A. Shutemov]

On Tue, Jun 25, 2019 at 09:04:39PM +0200, Thomas Gleixner wrote:
> On Thu, 20 Jun 2019, Kirill A. Shutemov wrote:
> > @@ -190,18 +190,18 @@ unsigned long __head __startup_64(unsigned long physaddr,
> >  		pgd[i + 0] = (pgdval_t)p4d + pgtable_flags;
> >  		pgd[i + 1] = (pgdval_t)p4d + pgtable_flags;
> >  
> > -		i = (physaddr >> P4D_SHIFT) % PTRS_PER_P4D;
> > -		p4d[i + 0] = (pgdval_t)pud + pgtable_flags;
> > -		p4d[i + 1] = (pgdval_t)pud + pgtable_flags;
> > +		i = physaddr >> P4D_SHIFT;
> > +		p4d[(i + 0) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
> > +		p4d[(i + 1) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
> >  	} else {
> >  		i = (physaddr >> PGDIR_SHIFT) % PTRS_PER_PGD;
> >  		pgd[i + 0] = (pgdval_t)pud + pgtable_flags;
> >  		pgd[i + 1] = (pgdval_t)pud + pgtable_flags;
> >  	}
> >  
> > -	i = (physaddr >> PUD_SHIFT) % PTRS_PER_PUD;
> > -	pud[i + 0] = (pudval_t)pmd + pgtable_flags;
> > -	pud[i + 1] = (pudval_t)pmd + pgtable_flags;
> > +	i = physaddr >> PUD_SHIFT;
> > +	pud[(i + 0) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
> > +	pud[(i + 1) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
> >  
> >  	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
> >  	/* Filter out unsupported __PAGE_KERNEL_* bits: */
> > @@ -211,8 +211,8 @@ unsigned long __head __startup_64(unsigned long physaddr,
> >  	pmd_entry +=  physaddr;
> >  
> >  	for (i = 0; i < DIV_ROUND_UP(_end - _text, PMD_SIZE); i++) {
> > -		int idx = i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;
> > -		pmd[idx] = pmd_entry + i * PMD_SIZE;
> > +		int idx = i + (physaddr >> PMD_SHIFT);;
> 
> double semicolon

Will fix.

> > +		pmd[idx % PTRS_PER_PMD] = pmd_entry + i * PMD_SIZE;
> 
> This part is functionally equivivalent. So what's the value of this change?

Precedence of operators were broken

	idx =  i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;

reads by compiler as

	idx = i + ((physaddr >> PMD_SHIFT) % PTRS_PER_PMD);

not as

	idx =  (i + (physaddr >> PMD_SHIFT)) % PTRS_PER_PMD;

Therefore 'idx' can become >= PTRS_PER_PMD.

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/boot/64: Fix crash if kernel images crosses page table boundary

[Thomas Gleixner]

On Tue, 25 Jun 2019, Kirill A. Shutemov wrote:
> On Tue, Jun 25, 2019 at 09:04:39PM +0200, Thomas Gleixner wrote:
> 
> > > +		pmd[idx % PTRS_PER_PMD] = pmd_entry + i * PMD_SIZE;
> > 
> > This part is functionally equivivalent. So what's the value of this change?
> 
> Precedence of operators were broken
> 
> 	idx =  i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;
> 
> reads by compiler as
> 
> 	idx = i + ((physaddr >> PMD_SHIFT) % PTRS_PER_PMD);
> 
> not as
> 
> 	idx =  (i + (physaddr >> PMD_SHIFT)) % PTRS_PER_PMD;
> 
> Therefore 'idx' can become >= PTRS_PER_PMD.

Indeed. Please mention it in the change log. I did not spot it right away.

Thanks,

	tglx

Re: [PATCH] x86/boot/64: Fix crash if kernel images crosses page table boundary

[Thomas Gleixner]

On Tue, 25 Jun 2019, Thomas Gleixner wrote:

> On Tue, 25 Jun 2019, Kirill A. Shutemov wrote:
> > On Tue, Jun 25, 2019 at 09:04:39PM +0200, Thomas Gleixner wrote:
> > 
> > > > +		pmd[idx % PTRS_PER_PMD] = pmd_entry + i * PMD_SIZE;
> > > 
> > > This part is functionally equivivalent. So what's the value of this change?
> > 
> > Precedence of operators were broken
> > 
> > 	idx =  i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;
> > 
> > reads by compiler as
> > 
> > 	idx = i + ((physaddr >> PMD_SHIFT) % PTRS_PER_PMD);
> > 
> > not as
> > 
> > 	idx =  (i + (physaddr >> PMD_SHIFT)) % PTRS_PER_PMD;
> > 
> > Therefore 'idx' can become >= PTRS_PER_PMD.
> 
> Indeed. Please mention it in the change log. I did not spot it right away.

I fixed it up already. No need to resend.

Thanks,

	tglx

[tip:x86/urgent] x86/boot/64: Fix crash if kernel image crosses page table boundary

[tip-bot for Kirill A. Shutemov]

Commit-ID:  81c7ed296dcd02bc0b4488246d040e03e633737a
Gitweb:     https://git.kernel.org/tip/81c7ed296dcd02bc0b4488246d040e03e633737a
Author:     Kirill A. Shutemov <kirill@shutemov.name>
AuthorDate: Thu, 20 Jun 2019 14:23:45 +0300
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 26 Jun 2019 07:25:09 +0200

x86/boot/64: Fix crash if kernel image crosses page table boundary

A kernel which boots in 5-level paging mode crashes in a small percentage
of cases if KASLR is enabled.

This issue was tracked down to the case when the kernel image unpacks in a
way that it crosses an 1G boundary. The crash is caused by an overrun of
the PMD page table in __startup_64() and corruption of P4D page table
allocated next to it. This particular issue is not visible with 4-level
paging as P4D page tables are not used.

But the P4D and the PUD calculation have similar problems.

The PMD index calculation is wrong due to operator precedence, which fails
to confine the PMDs in the PMD array on wrap around.

The P4D calculation for 5-level paging and the PUD calculation calculate
the first index correctly, but then blindly increment it which causes the
same issue when a kernel image is located across a 512G and for 5-level
paging across a 46T boundary.

This wrap around mishandling was introduced when these parts moved from
assembly to C.

Restore it to the correct behaviour.

Fixes: c88d71508e36 ("x86/boot/64: Rewrite startup_64() in C")
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20190620112345.28833-1-kirill.shutemov@linux.intel.com

---
 arch/x86/kernel/head64.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 16b1cbd3a61e..7df5bce4e1be 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -190,18 +190,18 @@ unsigned long __head __startup_64(unsigned long physaddr,
 		pgd[i + 0] = (pgdval_t)p4d + pgtable_flags;
 		pgd[i + 1] = (pgdval_t)p4d + pgtable_flags;
 
-		i = (physaddr >> P4D_SHIFT) % PTRS_PER_P4D;
-		p4d[i + 0] = (pgdval_t)pud + pgtable_flags;
-		p4d[i + 1] = (pgdval_t)pud + pgtable_flags;
+		i = physaddr >> P4D_SHIFT;
+		p4d[(i + 0) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
+		p4d[(i + 1) % PTRS_PER_P4D] = (pgdval_t)pud + pgtable_flags;
 	} else {
 		i = (physaddr >> PGDIR_SHIFT) % PTRS_PER_PGD;
 		pgd[i + 0] = (pgdval_t)pud + pgtable_flags;
 		pgd[i + 1] = (pgdval_t)pud + pgtable_flags;
 	}
 
-	i = (physaddr >> PUD_SHIFT) % PTRS_PER_PUD;
-	pud[i + 0] = (pudval_t)pmd + pgtable_flags;
-	pud[i + 1] = (pudval_t)pmd + pgtable_flags;
+	i = physaddr >> PUD_SHIFT;
+	pud[(i + 0) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
+	pud[(i + 1) % PTRS_PER_PUD] = (pudval_t)pmd + pgtable_flags;
 
 	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
 	/* Filter out unsupported __PAGE_KERNEL_* bits: */
@@ -211,8 +211,9 @@ unsigned long __head __startup_64(unsigned long physaddr,
 	pmd_entry +=  physaddr;
 
 	for (i = 0; i < DIV_ROUND_UP(_end - _text, PMD_SIZE); i++) {
-		int idx = i + (physaddr >> PMD_SHIFT) % PTRS_PER_PMD;
-		pmd[idx] = pmd_entry + i * PMD_SIZE;
+		int idx = i + (physaddr >> PMD_SHIFT);
+
+		pmd[idx % PTRS_PER_PMD] = pmd_entry + i * PMD_SIZE;
 	}
 
 	/*