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: <E4AF8E4C-A307-4AE7-85AA-F579D5BFDBDD@fb.com>
Date:   Tue, 17 Sep 2019 23:28:38 +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 12:01 PM, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> On Tue, Sep 17, 2019 at 11:49 AM Song Liu <songliubraving@...com> wrote:
>> 
>> 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;
> 
> I don't think we ever care about the low bits of the address below the
> page mask, so this one probably wouldn't make any difference.
> 
> To match the other cases, I'd make it just a plain
> 
>        WARN_ON_ONCE(start & ~PAGE_MASK));
> 
> and leave it at that. The existing commit added the warning, but then
> just made the code work despite it all.  I'd continue that same logic.
> 
>>                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;
> 
> Again, I think to match the other cases, I'd just do
> 
> -                       addr += PMD_SIZE;
> +                       WARN_ON_ONCE(addr & ~PMD_MASK);
> +                       addr = round_up(addr + 1, PMD_SIZE);
> 
> which now admittedly clones too much, but _does_ clone the requested range.
> 
> But maybe it really doesn't matter, since this condition just
> shouldn't happen in the first place.
> 
> And arguably, the "clone more than requested" issue is true, and maybe
> your "warn and refuse to clone by returning" is the right thing to do.
> 
> So I have very few strong opinions in this area, I just reacted to
> looking at the patch that it didn't seem to cover all the cases.

How about we just do:

diff --git i/arch/x86/mm/pti.c w/arch/x86/mm/pti.c
index b196524759ec..0437f65250db 100644
--- i/arch/x86/mm/pti.c
+++ w/arch/x86/mm/pti.c
@@ -341,6 +341,7 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
                }

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

So it is a "warn and continue" check just for unaligned PMD address. 

Thanks,
Song