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);
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.
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().
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
linux-stable-mirror@lists.linaro.org
-
Dave Hansen -
Kirill A. Shutemov -
Yuntao Wang