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: [email protected]
Signed-off-by: Yuntao Wang <[email protected]>
---
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 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().
--
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