linux-kernel - Re: [PATCH v2] x86/mm/pti: remove dead logic during user pagetable population

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]

Message-ID: <alpine.DEB.2.20.1801071303540.2094@nanos>
Date:   Sun, 7 Jan 2018 13:05:15 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
cc:     Jike Song <albcamus@...il.com>, linux-kernel@...r.kernel.org,
        David Woodhouse <dwmw@...zon.co.uk>,
        Alan Cox <gnomes@...rguk.ukuu.org.uk>,
        Jiri Koshina <jikos@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Andi Lutomirski <luto@...capital.net>,
        Andi Kleen <ak@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Paul Turner <pjt@...gle.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Greg KH <gregkh@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Kees Cook <keescook@...gle.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2] x86/mm/pti: remove dead logic during user pagetable
 population

On Sun, 7 Jan 2018, Borislav Petkov wrote:
> On Sun, Jan 07, 2018 at 06:33:17PM +0800, Jike Song wrote:
> > Look at one of the code snippets:
> > 
> >     162 if (pgd_none(*pgd)) {
> >     163         unsigned long new_p4d_page = __get_free_page(gfp);
> >     164         if (!new_p4d_page)
> >     165                 return NULL;
> >     166
> >     167         if (pgd_none(*pgd)) {
> >     168                 set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page)));
> >     169                 new_p4d_page = 0;
> >     170         }
> >     171         if (new_p4d_page)
> >     172                 free_page(new_p4d_page);
> >     173 }
> > 
> > There can't be any difference between two pgd_none(*pgd) at L162 and L167,
> > so it's always false at L171.
> 
> I think this is a remnant from the kaiser version which did this:
> 
>         if (pud_none(*pud)) {
>                 unsigned long new_pmd_page = __get_free_page(gfp);
>                 if (!new_pmd_page)
>                         return NULL;
>                 spin_lock(&shadow_table_allocation_lock);
>                 if (pud_none(*pud))
>                         set_pud(pud, __pud(_KERNPG_TABLE | __pa(new_pmd_page)));
>                 else
>                         free_page(new_pmd_page);
>                 spin_unlock(&shadow_table_allocation_lock);
>         }
> 
> I was wondering too, why the duplicated checks.
> 
> Which has this explanation about the need for the locking:
> 
> /*
>  * At runtime, the only things we map are some things for CPU
>  * hotplug, and stacks for new processes.  No two CPUs will ever
>  * be populating the same addresses, so we only need to ensure
>  * that we protect between two CPUs trying to allocate and
>  * populate the same page table page.
>  *
>  * Only take this lock when doing a set_p[4um]d(), but it is not
>  * needed for doing a set_pte().  We assume that only the *owner*
>  * of a given allocation will be doing this for _their_
>  * allocation.
>  *
>  * This ensures that once a system has been running for a while
>  * and there have been stacks all over and these page tables
>  * are fully populated, there will be no further acquisitions of
>  * this lock.
>  */
> static DEFINE_SPINLOCK(shadow_table_allocation_lock);
> 
> Now I have my suspicions why that's not needed anymore upstream but I'd
> let tglx explain better.

We got rid of all that runtime mapping stuff and the functions are only
called from pti_init(). So the locking and therefor the tests above are not
needed anymore. While at it we should mark all those function __init.

Thanks,

	tglx