linux-kernel - Re: [PATCH] x86/mm: Handle physical-virtual alignment mismatch in phys_p4d

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 23 Jun 2019 13:27:48 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
cc:     Dave Hansen <dave.hansen@...el.com>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
        linux-kernel@...r.kernel.org,
        Kyle Pelton <kyle.d.pelton@...el.com>,
        Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH] x86/mm: Handle physical-virtual alignment mismatch in
 phys_p4d_init()

On Fri, 21 Jun 2019, Kirill A. Shutemov wrote:
> On Thu, Jun 20, 2019 at 02:42:55PM +0000, Dave Hansen wrote:
> > On 6/20/19 4:22 AM, Kirill A. Shutemov wrote:
> > > The commit relaxes KASLR alignment requirements and it can lead to
> > > mismatch bentween 'i' and 'p4d_index(vaddr)' inside the loop in
> > > phys_p4d_init(). The mismatch in its turn leads to clearing wrong p4d
> > > entry and eventually to the oops.
> > 
> > Just curious, but what does it relax the requirement to and from?
> > 
> > I'm just not clearly spotting the actual bug.
> 
> Before the commit PAGE_OFFSET is always aligned to P4D_SIZE if we boot in
> 5-level paging mode. But now only PUD_SIZE alignment is guaranteed.
> 
> For phys_p4d_init() it means that paddr_next after the first iteration
> can belong to the same p4d entry.
> 
> In the case I was able to reproduce the vaddr on the first iteration is
> 0xff4228027fe00000 and paddr is 0x33fe00000. On the second iteration vaddr
> becomes 0xff42287f40000000 and paddr is 0x8000000000. The vaddr in both
> cases bolong to the same p4d entry.
> 
> It screws up 'i' count: we miss the last entry in the page table
> completely.  And it confuses the branch under 'paddr >= paddr_end'
> condition: the p4d entry can be cleared where must not to.
> 
> The patch makes phys_p4d_init() work like __kernel_physical_mapping_init()
> which deals with phys-virt mismatch already.
> 
> I hope this explanation makes any sense :P

Yes, and it should have been in the changelog right away. It's much more
useful than the 63 lines of untrimmed backtrace noise, which should be
condensed to the real valuable information:

  WARNING: CPU: 0 PID: 0 at arch/x86/mm/init_64.c:87 phys_p4d_init+0x1d4/0x1ea
  RIP: 0010:phys_p4d_init+0x1d4/0x1ea
  Call Trace:
   __kernel_physical_mapping_init+0x10a/0x35c
   kernel_physical_mapping_init+0xe/0x10
   init_memory_mapping+0x1aa/0x3b0
   init_range_memory_mapping+0xc8/0x116
   init_mem_mapping+0x225/0x2eb
   setup_arch+0x6ff/0xcf5
   start_kernel+0x64/0x53b
   ? copy_bootdata+0x1f/0xce
   x86_64_start_reservations+0x24/0x26
   x86_64_start_kernel+0x8a/0x8d
   secondary_startup_64+0xb6/0xc0

which causes later:

  BUG: unable to handle page fault for address: ff484d019580eff8
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  BAD
  Oops: 0000 [#1] SMP NOPTI
  RIP: 0010:fill_pud+0x13/0x130
  Call Trace:
   set_pte_vaddr_p4d+0x2e/0x50
   set_pte_vaddr+0x6f/0xb0
   __native_set_fixmap+0x28/0x40
   native_set_fixmap+0x39/0x70
   register_lapic_address+0x49/0xb6
   early_acpi_boot_init+0xa5/0xde
   setup_arch+0x944/0xcf5
   start_kernel+0x64/0x53b

Hmm?

Thanks,

	tglx