linux-kernel - Re: [BUG] msr-trace.h:42 suspicious rcu_dereference

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: <20161121154104.GA3124@twins.programming.kicks-ass.net>
Date:   Mon, 21 Nov 2016 16:41:04 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Steven Rostedt <rostedt@...dmis.org>, Jiri Olsa <jolsa@...hat.com>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Josh Triplett <josh@...htriplett.org>,
        Andi Kleen <andi@...stfloor.org>,
        Jan Stancek <jstancek@...hat.com>
Subject: Re: [BUG] msr-trace.h:42 suspicious rcu_dereference_check() usage!

On Mon, Nov 21, 2016 at 04:35:38PM +0100, Borislav Petkov wrote:
> On Mon, Nov 21, 2016 at 03:37:16PM +0100, Peter Zijlstra wrote:
> > Yeah, but this one does a printk() when it hits the contidion it checks
> > for, so not tracing it would be fine I think.
> 
> FWIW, there's already:
> 
> static inline void notrace
> native_write_msr(unsigned int msr, u32 low, u32 high)
> {
>         __native_write_msr_notrace(msr, low, high);
>         if (msr_tracepoint_active(__tracepoint_write_msr))
>                 do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> }
> 
> so could be mirrored.

Yeah, I know. I caused that to appear ;-)

> > Also, Boris, why do we need to redo that rdmsr until we see that bit
> > set? Can't we simply do the rdmsr once and then be done with it?
> 
> Hmm, so the way I read the definition of those bits -
> K8_INTP_C1E_ACTIVE_MASK - it looks like they get set when the cores
> enter halt:
> 
> 28 C1eOnCmpHalt: C1E on chip multi-processing halt. Read-write.
> 
> 1=When all cores of a processor have entered the halt state, the processor
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> generates an IO cycle as specified by IORd, IOMsgData, and IOMsgAddr.
> When this bit is set, SmiOnCmpHalt and IntPndMsg must be 0, otherwise
> the behavior is undefined. For revision DA-C, this bit is only supported
> for dual-core processors. For revision C3 and E, this bit is supported
> for any number of cores. See 2.4.3.3.3 [Hardware Initiated C1E].
> 
> 27 SmiOnCmpHalt: SMI on chip multi-processing halt. Read-write.
> 
> 1=When all cores of the processor have entered the halt state, the
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> processor generates an SMI-trigger IO cycle as specified by IORd,
> IOMsgData, and IOMsgAddr. When this bit is set C1eOnCmpHalt and
> IntPndMsg must be 0, otherwise the behavior is undefined. The status is
> stored in SMMFEC4[SmiOnCmpHaltSts]. See 2.4.3.3.1 [SMI Initiated C1E].
> 
> But the thing is, we do it only once until amd_e400_c1e_detected is true.
> 
> FWIW, I'd love it if we could do
> 
> 	if (cpu_has_bug(c, X86_BUG_AMD_APIC_C1E))
> 
> but I'm afraid we need to look at that MSR at least once until we set
> the boolean.
> 
> I could ask if that is really the case and whether we can detect it
> differently...
> 
> In any case, I have a box here in case you want me to test patches.

Right, I just wondered about the !c1e present case. In that case we'll
forever read the msr in the hope of finding that bit set, and we never
will.

Don't care too much, just idle curiousity. Let me do the rdmsr_notrace()
thing.