There are two issues in phys_p4d_init():

- The __kernel_physical_mapping_init() does not do boundary-checking for
  paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
  not do bounds checking either, so if the physical memory to be mapped is
  large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
  beginning entry of the P4D table and its data will be overwritten.

- The for loop body will be executed only when 'vaddr < vaddr_end'
  evaluates to true, but if that condition is true, 'paddr >= paddr_end'
  will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
  never be executed and become dead code.

To fix these issues, use 'i < PTRS_PER_P4D' instead of 'vaddr < vaddr_end'
as the for loop condition, this also make it more consistent with the logic
of the phys_{pud,pmt,pte}_init() functions.

Fixes: 432c833218dd ("x86/mm: Handle physical-virtual alignment mismatch in phys_p4d_init()")
Cc: stable@vger.kernel.org
Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 arch/x86/mm/init_64.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 8779d6be6a49..e718c9b3f539 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -674,18 +674,18 @@ static unsigned long __meminit
 phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 	      unsigned long page_size_mask, pgprot_t prot, bool init)
 {
-	unsigned long vaddr, vaddr_end, vaddr_next, paddr_next, paddr_last;
-
-	paddr_last = paddr_end;
-	vaddr = (unsigned long)__va(paddr);
-	vaddr_end = (unsigned long)__va(paddr_end);
+	unsigned long vaddr, vaddr_next, paddr_next, paddr_last;
+	int i;
 
 	if (!pgtable_l5_enabled())
 		return phys_pud_init((pud_t *) p4d_page, paddr, paddr_end,
 				     page_size_mask, prot, init);
 
-	for (; vaddr < vaddr_end; vaddr = vaddr_next) {
-		p4d_t *p4d = p4d_page + p4d_index(vaddr);
+	paddr_last = paddr_end;
+	vaddr = (unsigned long)__va(paddr);
+
+	for (i = p4d_index(vaddr); i < PTRS_PER_P4D; i++, vaddr = vaddr_next) {
+		p4d_t *p4d = p4d_page + i;
 		pud_t *pud;
 
 		vaddr_next = (vaddr & P4D_MASK) + P4D_SIZE;
@@ -704,13 +704,13 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
 
 		if (!p4d_none(*p4d)) {
 			pud = pud_offset(p4d, 0);
-			paddr_last = phys_pud_init(pud, paddr, __pa(vaddr_end),
+			paddr_last = phys_pud_init(pud, paddr, paddr_end,
 					page_size_mask, prot, init);
 			continue;
 		}
 
 		pud = alloc_low_page();
-		paddr_last = phys_pud_init(pud, paddr, __pa(vaddr_end),
+		paddr_last = phys_pud_init(pud, paddr, paddr_end,
 					   page_size_mask, prot, init);
 
 		spin_lock(&init_mm.page_table_lock);
-- 
2.36.0

On Thu, Jun 16, 2022 at 09:55:10PM +0800, Yuntao Wang wrote:
> There are two issues in phys_p4d_init():
> 
> - The __kernel_physical_mapping_init() does not do boundary-checking for
>   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
>   not do bounds checking either, so if the physical memory to be mapped is
>   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
>   beginning entry of the P4D table and its data will be overwritten.
> 
> - The for loop body will be executed only when 'vaddr < vaddr_end'
>   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
>   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
>   never be executed and become dead code.
> 
> To fix these issues, use 'i < PTRS_PER_P4D' instead of 'vaddr < vaddr_end'
> as the for loop condition, this also make it more consistent with the logic
> of the phys_{pud,pmt,pte}_init() functions.

Hm. I don't see why you changed phys_p4d_init(), but not
__kernel_physical_mapping_init(). It does exactly the same thing, just
pgd_index() is hidden a bit deeper than p4d_index().

-- 
 Kirill A. Shutemov

On Sat, 18 Jun 2022 03:22:20 +0300, Kirill A. Shutemov wrote:
> On Thu, Jun 16, 2022 at 09:55:10PM +0800, Yuntao Wang wrote:
> > There are two issues in phys_p4d_init():
> > 
> > - The __kernel_physical_mapping_init() does not do boundary-checking for
> >   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
> >   not do bounds checking either, so if the physical memory to be mapped is
> >   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
> >   beginning entry of the P4D table and its data will be overwritten.
> > 
> > - The for loop body will be executed only when 'vaddr < vaddr_end'
> >   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
> >   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
> >   never be executed and become dead code.
> > 
> > To fix these issues, use 'i < PTRS_PER_P4D' instead of 'vaddr < vaddr_end'
> > as the for loop condition, this also make it more consistent with the logic
> > of the phys_{pud,pmt,pte}_init() functions.
> 
> Hm. I don't see why you changed phys_p4d_init(), but not
> __kernel_physical_mapping_init(). It does exactly the same thing, just
> pgd_index() is hidden a bit deeper than p4d_index().

The reason I chose to change phys_p4d_init() is that:

- Currently the 'if (paddr >= paddr_end) {}' block in phys_p4d_init() is
  dead code, changing __kernel_physical_mapping_init() does not fix that.

- Changing phys_p4d_init() to the 'for (i < PTRS_PER_P4D) {}' form makes
  it more consistent with phys_pud/pmt/pte_init() as they are all using
  the 'for (i < PTRS_PER_PUD/PMD/PTE) {}' forms. Meanwhile, this change
  also fixes the dead code issue.

thanks,

Yuntao Wang

On 6/16/22 06:55, Yuntao Wang wrote:
> There are two issues in phys_p4d_init():
> 
> - The __kernel_physical_mapping_init() does not do boundary-checking for
>   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
>   not do bounds checking either, so if the physical memory to be mapped is
>   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
>   beginning entry of the P4D table and its data will be overwritten.
> 
> - The for loop body will be executed only when 'vaddr < vaddr_end'
>   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
>   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
>   never be executed and become dead code.

Could you explain a bit how you found this?  Was this encountered in
practice and debugged or was it found by inspection?

On Thu, 16 Jun 2022 07:02:56 -0700, Dave Hansen wrote:
> On 6/16/22 06:55, Yuntao Wang wrote:
> > There are two issues in phys_p4d_init():
> >
> > - The __kernel_physical_mapping_init() does not do boundary-checking for
> >   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
> >   not do bounds checking either, so if the physical memory to be mapped is
> >   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
> >   beginning entry of the P4D table and its data will be overwritten.
> >
> > - The for loop body will be executed only when 'vaddr < vaddr_end'
> >   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
> >   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
> >   never be executed and become dead code.
>
> Could you explain a bit how you found this?  Was this encountered in
> practice and debugged or was it found by inspection?

I found it by inspection.

On 6/16/22 07:15, Yuntao Wang wrote:
> On Thu, 16 Jun 2022 07:02:56 -0700, Dave Hansen wrote:
>> On 6/16/22 06:55, Yuntao Wang wrote:
>>> There are two issues in phys_p4d_init():
>>>
>>> - The __kernel_physical_mapping_init() does not do boundary-checking for
>>>   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
>>>   not do bounds checking either, so if the physical memory to be mapped is
>>>   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
>>>   beginning entry of the P4D table and its data will be overwritten.
>>>
>>> - The for loop body will be executed only when 'vaddr < vaddr_end'
>>>   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
>>>   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
>>>   never be executed and become dead code.
>> Could you explain a bit how you found this?  Was this encountered in
>> practice and debugged or was it found by inspection?
> I found it by inspection.

Dare I ask how this was tested?

On Thu, 16 Jun 2022 07:20:40 -0700, Dave Hansen wrote:
> On 6/16/22 07:15, Yuntao Wang wrote:
> > On Thu, 16 Jun 2022 07:02:56 -0700, Dave Hansen wrote:
> >> On 6/16/22 06:55, Yuntao Wang wrote:
> >>> There are two issues in phys_p4d_init():
> >>>
> >>> - The __kernel_physical_mapping_init() does not do boundary-checking for
> >>>   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
> >>>   not do bounds checking either, so if the physical memory to be mapped is
> >>>   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
> >>>   beginning entry of the P4D table and its data will be overwritten.
> >>>
> >>> - The for loop body will be executed only when 'vaddr < vaddr_end'
> >>>   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
> >>>   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
> >>>   never be executed and become dead code.
> >> Could you explain a bit how you found this?  Was this encountered in
> >> practice and debugged or was it found by inspection?
> > I found it by inspection.
>
> Dare I ask how this was tested?

Due to some limitations, I didn't test the changes thoroughly, I just built
the kernel and booted it in QEMU.

Considering that the patch was not fully tested, I spent a lot of time
reviewing the code I changed and tried my best to make it correct.