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: <9a9c0817-9ebb-524f-44df-176a15ea3fca@intel.com>
Date:   Thu, 26 Mar 2020 09:38:40 +0800
From:   Xiaoyao Li <xiaoyao.li@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        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>
Subject: Re: [PATCH v6 8/8] kvm: vmx: virtualize split lock detection

On 3/25/2020 9:41 AM, Thomas Gleixner wrote:
> Xiaoyao Li <xiaoyao.li@...el.com> writes:
>> On 3/25/2020 8:40 AM, Thomas Gleixner wrote:
>>>>    		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?
>>
>> So we need to reach an agreement on whether we need a state that host
>> turns it off but feature is available to be exposed to guest.
> 
> There is a very simple agreement:
> 
>    If the host turns it off, then it is not available at all
> 
>    If the host sets 'warn' then this applies to everything
> 
>    If the host sets 'fatal' then this applies to everything
> 
> Make it simple and consistent.

OK. you are the boss.

>>>> +	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.
>>
>> Can you image the case that both host and guest set sld_state == sld_warn.
>>
>> 1. There is guest userspace thread causing split lock.
>> 2. It sets TIF_SLD for the thread in guest, and clears SLD bit to re-
>> execute the instruction in guest.
>> 3. Then it still causes #AC since hardware SLD is not cleared. In host
>> kvm, we call handle_user_split_lock() that sets TIF_SLD for this VMM
>> thread, and clears hardware SLD bit. Then it enters guest and re-execute
>> the instruction.
>> 4. In guest, it schedules to another thread without TIF_SLD being set.
>> it sets the SLD bit to detect the split lock for this thread. So for
>> this purpose, we need to turn sld back on for the VMM thread, otherwise
>> this guest vcpu cannot catch split lock any more.
> 
> If you really want to address that scenario, then why are you needing
> any of those completely backwards interfaces at all?
> 
> Just because your KVM exception trap uses the host handling function
> which sets TIF_SLD?
>   

Yes. just because KVM use the host handling function.

If you disallow me to touch codes out of kvm. It can be achieved with 
something like in v2:
https://lore.kernel.org/kvm/20200203151608.28053-1-xiaoyao.li@intel.com/

Obviously re-use TIF_SLD flag to automatically switch MSR_TEST_CTRL.SLD 
bit when switch to/from vcpu thread is better.

And to virtualize SLD feature as full as possible for guest, we have to 
implement the backwards interface. If you really don't want that 
interface, we have to write code directly in kvm to modify TIF_SLD flag 
and MSR_TEST_CTRL.SLD bit.