linux-kernel - Re: [PATCHv2] arm64/cpufeature: don't use mutex in bringup path

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] [day] [month] [year] [list]

Message-ID: <20170511175357.GA29929@leverpostej>
Date:   Thu, 11 May 2017 18:53:58 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     Suzuki K Poulose <Suzuki.Poulose@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        bigeasy@...utronix.de, catalin.marinas@....com,
        peterz@...radead.org, tglx@...utronix.de, will.deacon@....com
Subject: Re: [PATCHv2] arm64/cpufeature: don't use mutex in bringup path

On Thu, May 11, 2017 at 05:08:19PM +0100, Marc Zyngier wrote:
> On 11/05/17 16:54, Suzuki K Poulose wrote:
> > On 11/05/17 16:37, Mark Rutland wrote:
> >> On Thu, May 11, 2017 at 04:15:38PM +0100, Suzuki K Poulose wrote:
> >>> On 11/05/17 16:01, Mark Rutland wrote:
> >>>> +static inline bool cpus_have_const_cap(int num)
> >>>> +{
> >>>> +	if (static_branch_likely(&arm64_const_caps_ready))
> >>>> +		return __cpus_have_const_cap(num);
> >>>> +	else
> >>>> +		return cpus_have_cap(num);
> >>>
> >>> We use cpus_have_const_cap() from hyp code, via has_vhe() and we could potentially
> >>> try to access unmapped kernel data from hyp if we fallback to cpus_have_cap().
> >>> However, it looks like we have already set arm64_const_caps_ready, so should not
> >>> hit it in practise. May be we could add a stricter version of the helper ?
> >>>
> >>> static inline cpus_have_const_cap_strict(int num)
> >>> {
> >>> 	BUG_ON(!static_branch_likely(&arm64_const_caps_ready);
> >>> 	return __cpus_have_const_cap(num);
> >>> }
> >>
> >> Just to check, is that the only user of cpus_have_const_cap() at hyp?
> > 
> > Uh, no we have one more, via system_supports_fpsimd() in __actvate_traps.
> 
> Indeed, and I'd definitely expect to see more of that trickling in (if
> only to deal with errata).
> 
> I'm OK with the BUG_ON version, TBH. It's not pretty, but it will be
> perfectly visible if it fires.

We can't make system_supports_fpsimd() BUG_ON(), because that will fire
the first time the boot CPU tries to switch thread, due to
fpsimd_thread_switch().

However, thinking about it, there's no risk that this code runs at hyp
before we've intialised the caps.

We initialise hyp from kvm_arch_init(), which is a module initcall. As
it's built-in, that's actually a device initcall, which happens long
after we've finalised the cpucaps.

So the v2 patch should be safe, though we can make that a little clearer
with the below, which I'll fold into v3.

Thanks,
Mark.

---->8----
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 5e19165..28bf4ea 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -24,6 +24,7 @@
 
 #include <linux/types.h>
 #include <linux/kvm_types.h>
+#include <asm/cpufeature.h>
 #include <asm/kvm.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmio.h>
@@ -356,8 +357,10 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
 {
        /*
         * Call initialization code, and switch to the full blown
-        * HYP code.
+        * HYP code. If the cpucaps haven't been finialized yet,
+        * something has gone very wrong, and hyp will crash and burn.
         */
+       BUG_ON(!static_branch_likely(&arm64_const_caps_ready));
        __kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr);
 }