| Message ID | 20240814124613.2632226-2-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | x86: Reduce code duplication on page table initialization | expand |
On 8/14/24 07:46, Kirill A. Shutemov wrote: > Calculation of 'next' virtual address doesn't protect against wrapping > to zero. It can result in page table corruption and hang. The > problematic case is possible if user sets high x86_mapping_info::offset. > > Replace manual 'next' calculation with p?d_addr_end() which handles > wrapping correctly. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Reviewed-by: Kai Huang <kai.huang@intel.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/mm/ident_map.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c > index 437e96fb4977..5872f3ee863c 100644 > --- a/arch/x86/mm/ident_map.c > +++ b/arch/x86/mm/ident_map.c > @@ -101,9 +101,7 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page, > pmd_t *pmd; > bool use_gbpage; > > - next = (addr & PUD_MASK) + PUD_SIZE; > - if (next > end) > - next = end; > + next = pud_addr_end(addr, end); > > /* if this is already a gbpage, this portion is already mapped */ > if (pud_leaf(*pud)) > @@ -154,10 +152,7 @@ static int ident_p4d_init(struct x86_mapping_info *info, p4d_t *p4d_page, > p4d_t *p4d = p4d_page + p4d_index(addr); > pud_t *pud; > > - next = (addr & P4D_MASK) + P4D_SIZE; > - if (next > end) > - next = end; > - > + next = p4d_addr_end(addr, end); > if (p4d_present(*p4d)) { > pud = pud_offset(p4d, 0); > result = ident_pud_init(info, pud, addr, next); > @@ -199,10 +194,7 @@ int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page, > pgd_t *pgd = pgd_page + pgd_index(addr); > p4d_t *p4d; > > - next = (addr & PGDIR_MASK) + PGDIR_SIZE; > - if (next > end) > - next = end; > - > + next = pgd_addr_end(addr, end); > if (pgd_present(*pgd)) { > p4d = p4d_offset(pgd, 0); > result = ident_p4d_init(info, p4d, addr, next);
On Wed, Aug 14 2024 at 15:46, Kirill A. Shutemov wrote: > Calculation of 'next' virtual address doesn't protect against wrapping > to zero. It can result in page table corruption and hang. The > problematic case is possible if user sets high x86_mapping_info::offset. So this should have a Fixes tag, right? > Replace manual 'next' calculation with p?d_addr_end() which handles > wrapping correctly. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Reviewed-by: Kai Huang <kai.huang@intel.com>
On Wed, Aug 14, 2024 at 09:25:35PM +0200, Thomas Gleixner wrote: > On Wed, Aug 14 2024 at 15:46, Kirill A. Shutemov wrote: > > Calculation of 'next' virtual address doesn't protect against wrapping > > to zero. It can result in page table corruption and hang. The > > problematic case is possible if user sets high x86_mapping_info::offset. > > So this should have a Fixes tag, right? Well, I guess we can add Fixes: e4630fdd4763 ("x86/power/64: Always create temporary identity mapping correctly") but the bug is not triggirable with current upstream code. It only wraps to zero if you touch top PGD entry. There's no such users in upstream. Only hibernate_64.c uses x86_mapping_info::offset and it works on direct mapping range which is not top PGD entry.
On Thu, Aug 15 2024 at 12:15, Kirill A. Shutemov wrote: > On Wed, Aug 14, 2024 at 09:25:35PM +0200, Thomas Gleixner wrote: >> On Wed, Aug 14 2024 at 15:46, Kirill A. Shutemov wrote: >> > Calculation of 'next' virtual address doesn't protect against wrapping >> > to zero. It can result in page table corruption and hang. The >> > problematic case is possible if user sets high x86_mapping_info::offset. >> >> So this should have a Fixes tag, right? > > Well, I guess we can add > > Fixes: e4630fdd4763 ("x86/power/64: Always create temporary identity mapping correctly") > > but the bug is not triggirable with current upstream code. > > It only wraps to zero if you touch top PGD entry. There's no such users in > upstream. Only hibernate_64.c uses x86_mapping_info::offset and it works > on direct mapping range which is not top PGD entry. Fair enough, but please mention that in the change log.
diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c index 437e96fb4977..5872f3ee863c 100644 --- a/arch/x86/mm/ident_map.c +++ b/arch/x86/mm/ident_map.c @@ -101,9 +101,7 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page, pmd_t *pmd; bool use_gbpage; - next = (addr & PUD_MASK) + PUD_SIZE; - if (next > end) - next = end; + next = pud_addr_end(addr, end); /* if this is already a gbpage, this portion is already mapped */ if (pud_leaf(*pud)) @@ -154,10 +152,7 @@ static int ident_p4d_init(struct x86_mapping_info *info, p4d_t *p4d_page, p4d_t *p4d = p4d_page + p4d_index(addr); pud_t *pud; - next = (addr & P4D_MASK) + P4D_SIZE; - if (next > end) - next = end; - + next = p4d_addr_end(addr, end); if (p4d_present(*p4d)) { pud = pud_offset(p4d, 0); result = ident_pud_init(info, pud, addr, next); @@ -199,10 +194,7 @@ int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page, pgd_t *pgd = pgd_page + pgd_index(addr); p4d_t *p4d; - next = (addr & PGDIR_MASK) + PGDIR_SIZE; - if (next > end) - next = end; - + next = pgd_addr_end(addr, end); if (pgd_present(*pgd)) { p4d = p4d_offset(pgd, 0); result = ident_p4d_init(info, p4d, addr, next);