linux-kernel - Re: [RESEND v4 2/2] KVM: VMX: Enable bus lock VM exit

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]

Date:   Mon, 26 Oct 2020 13:31:36 +0800
From:   Chenyi Qiang <chenyi.qiang@...el.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        "Li, Xiaoyao" <xiaoyao.li@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND v4 2/2] KVM: VMX: Enable bus lock VM exit



On 10/21/2020 6:19 AM, Sean Christopherson wrote:
> On Mon, Oct 12, 2020 at 11:35:42AM +0800, Chenyi Qiang wrote:
>> @@ -6138,6 +6149,26 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>   	return 0;
>>   }
>>   
>> +static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>> +{
>> +	int ret = __vmx_handle_exit(vcpu, exit_fastpath);
>> +
>> +	/*
>> +	 * Even when current exit reason is handled by KVM internally, we
>> +	 * still need to exit to user space when bus lock detected to inform
>> +	 * that there is a bus lock in guest.
>> +	 */
>> +	if (to_vmx(vcpu)->exit_reason.bus_lock_detected) {
>> +		if (ret > 0)
>> +			vcpu->run->exit_reason = KVM_EXIT_BUS_LOCK;
>> +		else
>> +			vcpu->run->flags |= KVM_RUN_BUS_LOCK;
> 
> This should always set flags.KVM_RUN_BUS_LOCK, e.g. so that userspace can
> always check flags.KVM_RUN_BUS_LOCK instead of having to check both the flag
> and the exit reason.  As is, it's really bad because the flag is undefined,
> which could teach userspace to do the wrong thing.
> 

Got it.

>> +		return 0;
>> +	}
>> +	vcpu->run->flags &= ~KVM_RUN_BUS_LOCK;
> 
> Hmm, I feel like explicitly clearing flags is should be unnecessary.  By
> that, I mean that's it's necessary in the current patch, bit I think we should
> figure out how to make that not be the case.  With the current approach, every
> chunk of code that needs to set a flag also needs to clear it, which increases
> the odds of missing a case and ending up with a flag in an undefined state.
> 
> The easiest way I can think of is to add another prep patch that zeros
> run->flags at the beginning of kvm_arch_vcpu_ioctl_run(), and changes
> post_kvm_run_save() to do:
> 
> 	if (is_smm(vcpu))
> 		kvm_run->flags |= KVM_RUN_X86_SMM;
> 
> Then this patch can omit clearing KVM_RUN_BUS_LOCK, and doesn't have to touch
> the SMM flag.
> 

Make sense, I will add the prep patch.

>> +	return ret;
>> +}
>> +
>>   /*
>>    * Software based L1D cache flush which is used when microcode providing
>>    * the cache control MSR is not loaded.