linux-kernel - Re: [GIT pull] x86/pti for 5.4-rc1

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: <CDB07BE3-6491-4159-89E7-FBA1631A01C3@fb.com>
Date:   Tue, 17 Sep 2019 18:48:46 +0000
From:   Song Liu <songliubraving@...com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        the arch/x86 maintainers <x86@...nel.org>
Subject: Re: [GIT pull] x86/pti for 5.4-rc1



> On Sep 17, 2019, at 11:13 AM, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> On Mon, Sep 16, 2019 at 6:38 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>> 
>>  - Handle unaligned addresses gracefully in pti_clone_pagetable(). Not an
>>    issue with current callers, but a correctness problem. Adds a warning
>>    so any caller which hands in an unaligned address gets pointed out
>>    clearly.
> 
> Hmm. I actually thing this one is incomplete.
> 
> Yes, it does it correctly now for the "those addresses are missing" cases.
> 
> But if somebody passes in an unaligned address, it still does the
> wrong thing for the
> 
>                if (pmd_large(*pmd) || level == PTI_CLONE_PMD) {
> 
> case. No?

I guess we need something like the following?

diff --git i/arch/x86/mm/pti.c w/arch/x86/mm/pti.c
index b196524759ec..7846916c3bcd 100644
--- i/arch/x86/mm/pti.c
+++ w/arch/x86/mm/pti.c
@@ -306,6 +306,8 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
 {
        unsigned long addr;

+       if (WARN_ON_ONCE(start & ~PAGE_MASK))
+               return;
        /*
         * Clone the populated PMDs which cover start to end. These PMD areas
         * can have holes.
@@ -341,6 +343,8 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
                }

                if (pmd_large(*pmd) || level == PTI_CLONE_PMD) {
+                       if (WARN_ON_ONCE(addr & ~PMD_MASK))
+                               return;
                        target_pmd = pti_user_pagetable_walk_pmd(addr);
                        if (WARN_ON(!target_pmd))
                                return;

> 
> I've pulled this, since the change is not _wrong_, but it does look
> incomplete to me. Am I missing something?
> 
> Also, it would have been lovely to have some background on how this
> was even noticed. The link in the commit message goes to the
> development thread, but that one doesn't have the original report from
> Song either.

I didn't really notice any issue. I was debugging an increase in iTLB 
miss rate, which was caused by splitting of kernel text page table, 
and was fixed by Thomas in:

https://lore.kernel.org/lkml/alpine.DEB.2.21.1908282355340.1938@nanos.tec.linutronix.de/

I mistakenly suspected the issue was caused by the pti code, and 
mistakenly proposed the first patch here. It turned out to be useful, 
but it is not related to the original issue. 

Thanks,
Song