linux-kernel - Re: [PATCH] x86/nmi: Fix some races in NMI uaccess

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:   Mon, 27 Aug 2018 19:10:35 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Rik van Riel <riel@...riel.com>
Cc:     Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        Borislav Petkov <bp@...en8.de>, Jann Horn <jannh@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Nadav Amit <nadav.amit@...il.com>
Subject: Re: [PATCH] x86/nmi: Fix some races in NMI uaccess

On Mon, Aug 27, 2018 at 6:31 PM, Rik van Riel <riel@...riel.com> wrote:
> On Mon, 2018-08-27 at 16:04 -0700, Andy Lutomirski wrote:
>
>> +++ b/arch/x86/mm/tlb.c
>> @@ -345,6 +345,9 @@ void switch_mm_irqs_off(struct mm_struct *prev,
>> struct mm_struct *next,
>>                */
>>               trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
>> TLB_FLUSH_ALL);
>>       } else {
>> +             /* Let NMI code know that CR3 may not match
>> expectations. */
>
> I don't get it. This is in the "ASID is up to date, do not
> need a TLB flush" path.
>
> In what case do we have a TLB that is fully up to date, but
> a CR3 that does not match expectations?
>
> Doesn't the CR3 check in nmi_uaccess_ok already catch the
> window of time where the CR3 has already been switched over
> to that of the next task?
>
> What is special about this path wrt nmi_uaccess_ok that is
> not also true for the need_flush branch right above it?
>
> What am I missing?

Nothing.  My patch is buggy.  ETOLITTLESLEEP.

I could drop this part of the patch entirely.  Or I could drop the
loaded_mm->pgd == __va(read_cr3_pa() check and instead make sure that
loaded_mm is NULL at any point at which loaded_mm might not match CR3.
The latter will be faster in any (hypothetical) virtualization
environment where CR3 reads trap.  I don't know if we have any such
cases where perf works and we care about performance, though.

--Andy