linux-kernel - Re: [PATCH v2 1/6] KVM: x86: Add function for vectoring error generation

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: <Z1nTp82wgSGe4AmV@google.com>
Date: Wed, 11 Dec 2024 10:02:15 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Ivan Orlov <iorlov@...zon.com>
Cc: bp@...en8.de, dave.hansen@...ux.intel.com, mingo@...hat.com, 
	pbonzini@...hat.com, shuah@...nel.org, tglx@...utronix.de, hpa@...or.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, x86@...nel.org, pdurrant@...zon.co.uk, 
	dwmw@...zon.co.uk
Subject: Re: [PATCH v2 1/6] KVM: x86: Add function for vectoring error generation

On Mon, Nov 11, 2024, Ivan Orlov wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f6900bec4874..f92740e7e107 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6452,6 +6452,7 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  	union vmx_exit_reason exit_reason = vmx->exit_reason;
>  	u32 vectoring_info = vmx->idt_vectoring_info;
>  	u16 exit_handler_index;
> +	gpa_t gpa;

I've gone back and forth on where to declare scoped varaibles, but in this case,
I think it makes sense to declare "gpa" inside the if-statement.  Making it
visible at the function scope when it's valid in a _super_ limited case is bound
to cause issues.

Of course, this code goes away by the end of the series, so that point is moot.
But on the other hand, declaring the variable in the if-statement is desirable
as the churn is precisely limited to the code that's being changed.

>  	/*
>  	 * Flush logged GPAs PML buffer, this will make dirty_bitmap more
> @@ -6550,19 +6551,10 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  	     exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
>  	     exit_reason.basic != EXIT_REASON_TASK_SWITCH &&
>  	     exit_reason.basic != EXIT_REASON_NOTIFY)) {
> -		int ndata = 3;
> +		gpa = exit_reason.basic == EXIT_REASON_EPT_MISCONFIG
> +		      ? vmcs_read64(GUEST_PHYSICAL_ADDRESS) : INVALID_GPA;

Again a moot point, but IMO using a ternary operator here makes it unnecessarily
difficult to see that gpa is valid if and only if the exit was an EPT misconfig.

		gpa_t gpa = INVALID_GPA;

		if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG)
			gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);


> -		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
> -		vcpu->run->internal.data[0] = vectoring_info;
> -		vcpu->run->internal.data[1] = exit_reason.full;
> -		vcpu->run->internal.data[2] = vmx_get_exit_qual(vcpu);
> -		if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) {
> -			vcpu->run->internal.data[ndata++] =
> -				vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> -		}
> -		vcpu->run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
> -		vcpu->run->internal.ndata = ndata;
> +		kvm_prepare_event_vectoring_exit(vcpu, gpa);
>  		return 0;
>  	}
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 83fe0a78146f..e338d583f48f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8828,6 +8828,28 @@ void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_prepare_emulation_failure_exit);
>  
> +void kvm_prepare_event_vectoring_exit(struct kvm_vcpu *vcpu, gpa_t gpa)
> +{
> +	u32 reason, intr_info, error_code;
> +	struct kvm_run *run = vcpu->run;
> +	u64 info1, info2;
> +	int ndata = 0;
> +
> +	kvm_x86_call(get_exit_info)(vcpu, &reason, &info1, &info2,
> +				    &intr_info, &error_code);
> +
> +	run->internal.data[ndata++] = info2;
> +	run->internal.data[ndata++] = reason;
> +	run->internal.data[ndata++] = info1;
> +	run->internal.data[ndata++] = (u64)gpa;

No need for the cast.

> +	run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
> +
> +	run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +	run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
> +	run->internal.ndata = ndata;
> +}
> +EXPORT_SYMBOL_GPL(kvm_prepare_event_vectoring_exit);
> +
>  static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>  {
>  	struct kvm *kvm = vcpu->kvm;
> -- 
> 2.43.0
>