linux-kernel - Re: [PATCH v6 8/8] kvm: vmx: virtualize split lock detection

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: <87eethz2p6.fsf@nanos.tec.linutronix.de>
Date:   Wed, 25 Mar 2020 01:40:53 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Xiaoyao Li <xiaoyao.li@...el.com>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>, hpa@...or.com,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Arvind Sankar <nivedita@...m.mit.edu>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v6 8/8] kvm: vmx: virtualize split lock detection

Xiaoyao Li <xiaoyao.li@...el.com> writes:
>  #ifdef CONFIG_CPU_SUP_INTEL
> +enum split_lock_detect_state {
> +	sld_off = 0,
> +	sld_warn,
> +	sld_fatal,
> +};
> +extern enum split_lock_detect_state sld_state __ro_after_init;
> +
> +static inline bool split_lock_detect_on(void)
> +{
> +	return sld_state != sld_off;
> +}

See previous reply.

> +void sld_msr_set(bool on)
> +{
> +	sld_update_msr(on);
> +}
> +EXPORT_SYMBOL_GPL(sld_msr_set);
> +
> +void sld_turn_back_on(void)
> +{
> +	sld_update_msr(true);
> +	clear_tsk_thread_flag(current, TIF_SLD);
> +}
> +EXPORT_SYMBOL_GPL(sld_turn_back_on);

First of all these functions want to be in a separate patch, but aside
of that they do not make any sense at all.

> +static inline bool guest_cpu_split_lock_detect_on(struct vcpu_vmx *vmx)
> +{
> +	return vmx->msr_test_ctrl & MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> +}
> +
>  static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -4725,12 +4746,13 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>  	case AC_VECTOR:
>  		/*
>  		 * Reflect #AC to the guest if it's expecting the #AC, i.e. has
> -		 * legacy alignment check enabled.  Pre-check host split lock
> -		 * support to avoid the VMREADs needed to check legacy #AC,
> -		 * i.e. reflect the #AC if the only possible source is legacy
> -		 * alignment checks.
> +		 * legacy alignment check enabled or split lock detect enabled.
> +		 * Pre-check host split lock support to avoid further check of
> +		 * guest, i.e. reflect the #AC if host doesn't enable split lock
> +		 * detection.
>  		 */
>  		if (!split_lock_detect_on() ||
> +		    guest_cpu_split_lock_detect_on(vmx) ||
>  		    guest_cpu_alignment_check_enabled(vcpu)) {

If the host has split lock detection disabled then how is the guest
supposed to have it enabled in the first place?

> @@ -6631,6 +6653,14 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	 */
>  	x86_spec_ctrl_set_guest(vmx->spec_ctrl, 0);
>  
> +	if (static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
> +	    guest_cpu_split_lock_detect_on(vmx)) {
> +		if (test_thread_flag(TIF_SLD))
> +			sld_turn_back_on();

This is completely inconsistent behaviour. The only way that TIF_SLD is
set is when the host has sld_state == sld_warn and the guest triggered
a split lock #AC.

'warn' means that the split lock event is registered and a printk
emitted and after that the task runs with split lock detection disabled.

It does not matter at all if the task triggered the #AC while in guest
or in host user space mode. Stop claiming that virt is special. The only
special thing about virt is, that it is using a different mechanism to
exit kernel mode. Aside of that from the kernel POV it is completely
irrelevant whether the task triggered the split lock in host user space
or in guest mode.

If the SLD mode is fatal, then the task is killed no matter what.

Please sit down and go through your patches and rethink every single
line instead of sending out yet another half baken and hastily cobbled
together pile.

To be clear, Patch 1 and 2 make sense on their own, so I'm tempted to
pick them up right now, but the rest is going to be 5.8 material no
matter what.

Thanks,

        tglx