* [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
@ 2022-06-16 13:55 Yuntao Wang
2022-06-16 14:02 ` Dave Hansen
2022-06-18 0:22 ` Kirill A. Shutemov
0 siblings, 2 replies; 7+ messages in thread
From: Yuntao Wang @ 2022-06-16 13:55 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov
Cc: H. Peter Anvin, Baoquan He, Kirill A. Shutemov, x86,
linux-kernel, Yuntao Wang, stable
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
2022-06-16 13:55 [PATCH] x86/mm: Fix possible index overflow when creating page table mapping Yuntao Wang
@ 2022-06-16 14:02 ` Dave Hansen
2022-06-16 14:15 ` Yuntao Wang
2022-06-18 0:22 ` Kirill A. Shutemov
1 sibling, 1 reply; 7+ messages in thread
From: Dave Hansen @ 2022-06-16 14:02 UTC (permalink / raw)
To: Yuntao Wang, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Thomas Gleixner, Ingo Molnar, Borislav Petkov
Cc: H. Peter Anvin, Baoquan He, Kirill A. Shutemov, x86,
linux-kernel, stable
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
2022-06-16 14:02 ` Dave Hansen
@ 2022-06-16 14:15 ` Yuntao Wang
2022-06-16 14:20 ` Dave Hansen
0 siblings, 1 reply; 7+ messages in thread
From: Yuntao Wang @ 2022-06-16 14:15 UTC (permalink / raw)
To: dave.hansen
Cc: bhe, bp, dave.hansen, hpa, kirill, linux-kernel, luto, mingo,
peterz, stable, tglx, x86, ytcoode
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
2022-06-16 14:15 ` Yuntao Wang
@ 2022-06-16 14:20 ` Dave Hansen
2022-06-17 6:38 ` Yuntao Wang
0 siblings, 1 reply; 7+ messages in thread
From: Dave Hansen @ 2022-06-16 14:20 UTC (permalink / raw)
To: Yuntao Wang
Cc: bhe, bp, dave.hansen, hpa, kirill, linux-kernel, luto, mingo,
peterz, stable, tglx, x86
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
2022-06-16 14:20 ` Dave Hansen
@ 2022-06-17 6:38 ` Yuntao Wang
0 siblings, 0 replies; 7+ messages in thread
From: Yuntao Wang @ 2022-06-17 6:38 UTC (permalink / raw)
To: dave.hansen
Cc: bhe, bp, dave.hansen, hpa, kirill, linux-kernel, luto, mingo,
peterz, stable, tglx, x86, ytcoode
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
2022-06-16 13:55 [PATCH] x86/mm: Fix possible index overflow when creating page table mapping Yuntao Wang
2022-06-16 14:02 ` Dave Hansen
@ 2022-06-18 0:22 ` Kirill A. Shutemov
2022-06-18 2:08 ` Yuntao Wang
1 sibling, 1 reply; 7+ messages in thread
From: Kirill A. Shutemov @ 2022-06-18 0:22 UTC (permalink / raw)
To: Yuntao Wang
Cc: Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, Baoquan He, x86,
linux-kernel, stable
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
2022-06-18 0:22 ` Kirill A. Shutemov
@ 2022-06-18 2:08 ` Yuntao Wang
0 siblings, 0 replies; 7+ messages in thread
From: Yuntao Wang @ 2022-06-18 2:08 UTC (permalink / raw)
To: kirill
Cc: bhe, bp, dave.hansen, hpa, linux-kernel, luto, mingo, peterz,
stable, tglx, x86, ytcoode
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-18 2:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-16 13:55 [PATCH] x86/mm: Fix possible index overflow when creating page table mapping Yuntao Wang
2022-06-16 14:02 ` Dave Hansen
2022-06-16 14:15 ` Yuntao Wang
2022-06-16 14:20 ` Dave Hansen
2022-06-17 6:38 ` Yuntao Wang
2022-06-18 0:22 ` Kirill A. Shutemov
2022-06-18 2:08 ` Yuntao Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.