Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448716

Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd, don't set the pgd entry until it's populated

Show all headers | View raw

On Fri, Jul 22, 2016 at 3:21 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@kernel.org> wrote:
>
>> On 07/21/2016 09:43 PM, Valdis.Kletnieks@vt.edu wrote:
>> >On Mon, 11 Jul 2016 13:53:36 -0700, Andy Lutomirski said:
>> >>This avoids pointless races in which another CPU or task might see a
>> >>partially populated global pgd entry.  These races should normally
>> >>be harmless, but, if another CPU propagates the entry via
>> >>vmalloc_fault and then populate_pgd fails (due to memory allocation
>> >>failure, for example), this prevents a use-after-free of the pgd
>> >>entry.
>> >>
>> >>Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> >>---
>> >> arch/x86/mm/pageattr.c | 9 ++++++---
>> >> 1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> >I just bisected a failure to boot down to this patch.  On my Dell Latitude
>> >laptop, it results in the kernel being loaded and then just basically sitting
>> >there dead in the water - as far as I can tell, it dies before the kernel
>> >ever gets going far enough to do any console I/O (even with ignore_loglevel).
>> >Nothing in /sys/fs/pstore either.  I admit not understanding the VM code
>> >at all, so I don't have a clue *why* this causes indigestion...
>> >
>> >CPU is an Intel Core i5-3340M in case that matters....
>> >
>>
>> How much memory do you have and what's your config?  My code is obviously
>> buggy, but I'm wondering why neither I nor the 0day bot caught this.
>>
>> The attached patch is compile-tested only.  (Even Thunderbird doesn't want
>> to send non-flowed text right now, sigh.)
>>
>> --Andy
>
>> From 6589ddf69a1369e1ecb95f0af489d90b980e256e Mon Sep 17 00:00:00 2001
>> Message-Id: <6589ddf69a1369e1ecb95f0af489d90b980e256e.1469165371.git.luto@kernel.org>
>> From: Andy Lutomirski <luto@kernel.org>
>> Date: Thu, 21 Jul 2016 22:22:02 -0700
>> Subject: [PATCH] x86/mm: Fix populate_pgd()
>>
>> I make an obvious error in populate_pgd() -- it would fail to correctly
>> populate the page tables when it allocated a new pud page.
>
> JFYI, on allnoconfig it gives:
>
>   arch/x86/mm/pageattr.c:1016:20: error: implicit declaration of function ‘pud_index’ [-Werror=implicit-function-declaration]

As it happens, my fix interacts badly with the steaming pile of crap
that is Linux's support for <4 page table levels.  Can you just revert
the offending patch and I'll redo it differently?

<rant>
Holy crap the pagetable structures and helpers suck.  We have "pgd_t
*" that could point to a top-level entry, a top-level table, or to
something else entirely if we have fewer than four levels.  And we
have pud_t * that ambiguously points to a table or to an entry or to a
pmd if the build is feeling daft.  We have a helper called
"pud_offset" that doesn't compute any sort of offset -- it *traverses*
one level of the table and only works if you pass it the kind of pgt_t
* that points to an entry (not a table).

This garbage (as evidenced by my bug and my failed attempt to fix it)
only works if you never have a low-level page table that isn't linked
into a higher-level page table, and it mostly requires you to do
everything exactly the way it was originally done so all the horrible
inline helpers don't get confused.

And AFAICT all of this was done to manually unroll a loop, and I bet
it never sped anything up measurably even on 386 or PPro.

Whenever some vendor releases a 5 level page table CPU, can we
*please* clean this up first?  We should have a type that points to a
table, a different type that points to an entry (or maybe not have
pointers to entries at all), and the levels should be referred to by
*number*.  When you need to traverse all the way down, you write a
*loop* instead of four bloody helper functions, some of which are
incomprehensibly no-ops on some kernels.  And if this means that, on
Intel, we have a silly branch in the inner loop because the bottom
level entry format is special, who cares?
</rant>

--Andy

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v5 00/32] virtually mapped stacks and thread_info cleanup Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 18/32] signal: Consolidate {TS,TLF}_RESTORE_SIGMASK code Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
    Re: [PATCH v5 18/32] signal: Consolidate {TS,TLF}_RESTORE_SIGMASK code Brian Gerst <brgerst@gmail.com> - 2016-07-12 14:00 +0200
      Re: [PATCH v5 18/32] signal: Consolidate {TS,TLF}_RESTORE_SIGMASK code Andy Lutomirski <luto@amacapital.net> - 2016-07-13 01:10 +0200
  [PATCH v5 08/32] dma-api: Teach the "DMA-from-stack" check about vmapped stacks Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 32/32] fork: Cache two thread stacks per cpu if CONFIG_VMAP_STACK is set Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 27/32] x86: Move thread_info into task_struct Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 31/32] sched: Free the stack early if CONFIG_THREAD_INFO_IN_TASK Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 23/32] printk: When dumping regs, show the stack, not thread_info Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 19/32] x86/smp: Remove stack_smp_processor_id() Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 20/32] x86/smp: Remove unnecessary initialization of thread_info::cpu Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 16/32] x86: Move uaccess_err and sig_on_uaccess_err to thread_struct Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:00 +0200
  [PATCH v5 04/32] x86/mm: Remove kernel_unmap_pages_in_pgd() and efi_cleanup_page_tables() Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 01/32] bluetooth: Switch SMP to crypto_cipher_encrypt_one() Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
    Re: [PATCH v5 01/32] bluetooth: Switch SMP to crypto_cipher_encrypt_one() Andy Lutomirski <luto@amacapital.net> - 2016-07-14 21:20 +0200
      Re: [PATCH v5 01/32] bluetooth: Switch SMP to crypto_cipher_encrypt_one() Marcel Holtmann <marcel@holtmann.org> - 2016-07-14 22:40 +0200
      Re: [PATCH v5 01/32] bluetooth: Switch SMP to  crypto_cipher_encrypt_one() David Miller <davem@davemloft.net> - 2016-07-14 22:50 +0200
  [PATCH v5 05/32] mm: Track NR_KERNEL_STACK in KiB instead of number of stacks Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 12/32] x86/dumpstack/64: Handle faults when printing the "Stack:" part of an OOPS Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 06/32] mm: Fix memcg stack accounting for sub-page stacks Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 03/32] x86/cpa: In populate_pgd, don't set the pgd entry until it's populated Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
    Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd, don't set the pgd entry until it's populated Valdis.Kletnieks@vt.edu - 2016-07-22 06:50 +0200
      Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd,  don't set the pgd entry until it's populated Andy Lutomirski <luto@kernel.org> - 2016-07-22 07:40 +0200
        Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd,  don't set the pgd entry until it's populated Ingo Molnar <mingo@kernel.org> - 2016-07-22 12:30 +0200
          Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd,  don't set the pgd entry until it's populated Andy Lutomirski <luto@amacapital.net> - 2016-07-22 20:30 +0200
            Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd,  don't set the pgd entry until it's populated Andy Lutomirski <luto@amacapital.net> - 2016-07-22 20:40 +0200
            Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd,  don't set the pgd entry until it's populated Ingo Molnar <mingo@kernel.org> - 2016-07-22 22:20 +0200
        Re: [kernel-hardening] [PATCH v5 03/32] x86/cpa: In populate_pgd, don't set the pgd entry until it's populated Nicolai Stange <nicstange@gmail.com> - 2016-07-23 17:00 +0200
  [PATCH v5 10/32] x86/dumpstack: Honor supplied @regs arg Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 02/32] x86/mm/hotplug: Don't remove PGD entries in remove_pagetable() Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 15/32] x86/mm: Improve stack-overflow #PF handling Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  [PATCH v5 11/32] x86/dumpstack: Try harder to get a call trace on stack overflow Andy Lutomirski <luto@kernel.org> - 2016-07-11 23:10 +0200
  Re: [PATCH v5 00/32] virtually mapped stacks and thread_info cleanup Herbert Xu <herbert@gondor.apana.org.au> - 2016-07-12 11:00 +0200
  Re: [PATCH v5 00/32] virtually mapped stacks and thread_info cleanup Christian Borntraeger <borntraeger@de.ibm.com> - 2016-07-13 11:00 +0200
    Re: [PATCH v5 00/32] virtually mapped stacks and thread_info cleanup Andy Lutomirski <luto@amacapital.net> - 2016-07-13 20:40 +0200
      Re: [PATCH v5 00/32] virtually mapped stacks and thread_info cleanup Christian Borntraeger <borntraeger@de.ibm.com> - 2016-07-13 21:00 +0200

csiph-web