linux-kernel - Re: [PATCH v2 5/7] x86/mm, tracing: Fix CR2 corruption

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:   Sat, 6 Jul 2019 18:27:28 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...nel.org>,
        Andrew Lutomirski <luto@...nel.org>,
        Peter Anvin <hpa@...or.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Juergen Gross <jgross@...e.com>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        He Zhe <zhe.he@...driver.com>,
        Joel Fernandes <joel@...lfernandes.org>, devel@...ukata.com
Subject: Re: [PATCH v2 5/7] x86/mm, tracing: Fix CR2 corruption

On Sat, 6 Jul 2019 14:41:22 -0700
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Fri, Jul 5, 2019 at 6:50 AM Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > Also; all previous attempts at fixing this have been about pushing the
> > read_cr2() earlier; notably:
> >
> >   0ac09f9f8cd1 ("x86, trace: Fix CR2 corruption when tracing page faults")
> >   d4078e232267 ("x86, trace: Further robustify CR2 handling vs tracing")  
> 
> I think both of those are because people - again - felt like tracing
> can validly corrupt CPU state, and then they fix up the symptoms
> rather than the cause.
> 
> Which I disagree with.
> 
> > And I'm thinking that with exception of this patch, the rest are
> > worthwhile cleanups regardless.  
> 
> I don't have any issues with the patches themselves, I agree that they
> are probably good on their own.
> 
> I *do* have issues with the "tracing can change CPU state so we need
> to deal with it" model, though. I think that mode of thinking is
> wrong. I don't believe tracing should ever change core CPU state and
> that be considered ok.
> 
> > Also; while looking at this, if we do continue with the C wrappers from
> > the very last patch, we can do horrible things like this on top and move
> > the read_cr2() back into C code.  
> 
> Again, I don't think that is the problem. I think it's a much more
> fundamental problem in thinking that core code should be changed
> because tracing is broken garbage and didn't do things right.
> 
> I see Eiichi's patch, and it makes me go "that looks better" - simply
> because it fixes the fundamental issue, rather than working around the
> symptoms.
>

We also have to deal with reading vmalloc'd data as that can fault too.
The perf ring buffer IIUC is vmalloc, so if perf records in one of
these locations, then the reading of the vmalloc area has a potential
to fault corrupting the CR2 register as well. Or have we changed
vmalloc to no longer fault?

-- Steve