[PATCH] KVM: Hyper-V: do not do hypercall userspace exits if SynIC is disabled

[PATCH] KVM: Hyper-V: do not do hypercall userspace exits if SynIC is disabled

[Paolo Bonzini]

If SynIC is disabled, there is nothing that userspace can do to
handle these exits; on the other hand, userspace probably will
not know about KVM_EXIT_HYPERV_HCALL and complain about it or
even exit.  Just prevent anything bad from happening by handling
the hypercall in KVM and returning an "invalid hypercall" code.

Fixes: 83326e43f27e9a8a501427a0060f8af519a39bb2
Cc: Andrey Smetanin <asmetanin@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/hyperv.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index b960d9ea171f..9e2becd20a59 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1237,14 +1237,17 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
 		break;
 	case HVCALL_POST_MESSAGE:
 	case HVCALL_SIGNAL_EVENT:
-		vcpu->run->exit_reason = KVM_EXIT_HYPERV;
-		vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
-		vcpu->run->hyperv.u.hcall.input = param;
-		vcpu->run->hyperv.u.hcall.params[0] = ingpa;
-		vcpu->run->hyperv.u.hcall.params[1] = outgpa;
-		vcpu->arch.complete_userspace_io =
+		if (vcpu_to_synic(vcpu)->active) {
+			vcpu->run->exit_reason = KVM_EXIT_HYPERV;
+			vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
+			vcpu->run->hyperv.u.hcall.input = param;
+			vcpu->run->hyperv.u.hcall.params[0] = ingpa;
+			vcpu->run->hyperv.u.hcall.params[1] = outgpa;
+			vcpu->arch.complete_userspace_io =
 				kvm_hv_hypercall_complete_userspace;
-		return 0;
+			return 0;
+		}
+		/* fall through */
 	default:
 		res = HV_STATUS_INVALID_HYPERCALL_CODE;
 		break;
-- 
1.8.3.1

Re: [PATCH] KVM: Hyper-V: do not do hypercall userspace exits if SynIC is disabled

[Roman Kagan]

On Tue, Mar 29, 2016 at 11:34:56AM +0200, Paolo Bonzini wrote:
> If SynIC is disabled, there is nothing that userspace can do to
> handle these exits; on the other hand, userspace probably will
> not know about KVM_EXIT_HYPERV_HCALL and complain about it or
> even exit.  Just prevent anything bad from happening by handling
> the hypercall in KVM and returning an "invalid hypercall" code.

I wonder if this has been encountered in real life or just found by code
inspection?

> Fixes: 83326e43f27e9a8a501427a0060f8af519a39bb2
> Cc: Andrey Smetanin <asmetanin@virtuozzo.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/hyperv.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index b960d9ea171f..9e2becd20a59 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1237,14 +1237,17 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>  		break;
>  	case HVCALL_POST_MESSAGE:
>  	case HVCALL_SIGNAL_EVENT:
> -		vcpu->run->exit_reason = KVM_EXIT_HYPERV;
> -		vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
> -		vcpu->run->hyperv.u.hcall.input = param;
> -		vcpu->run->hyperv.u.hcall.params[0] = ingpa;
> -		vcpu->run->hyperv.u.hcall.params[1] = outgpa;
> -		vcpu->arch.complete_userspace_io =
> +		if (vcpu_to_synic(vcpu)->active) {
> +			vcpu->run->exit_reason = KVM_EXIT_HYPERV;
> +			vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
> +			vcpu->run->hyperv.u.hcall.input = param;
> +			vcpu->run->hyperv.u.hcall.params[0] = ingpa;
> +			vcpu->run->hyperv.u.hcall.params[1] = outgpa;
> +			vcpu->arch.complete_userspace_io =
>  				kvm_hv_hypercall_complete_userspace;
> -		return 0;
> +			return 0;
> +		}
> +		/* fall through */

I'd rather put it the other way around, with explicit error path and a
comment:


		/* don't bother userspace if it has no way to handle it */
		if (!vcpu_to_synic(vcpu)->active) {
			res = HV_STATUS_INVALID_HYPERCALL_CODE;
			break;
		}

but that's just nitpicking, so

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

[ Andrey has left the company so I'm not sure he even receives the
messages sent to this address ]

Thanks,
Roman.

Re: [PATCH] KVM: Hyper-V: do not do hypercall userspace exits if SynIC is disabled

[Paolo Bonzini]

On 29/03/2016 12:48, Roman Kagan wrote:
> On Tue, Mar 29, 2016 at 11:34:56AM +0200, Paolo Bonzini wrote:
>> If SynIC is disabled, there is nothing that userspace can do to
>> handle these exits; on the other hand, userspace probably will
>> not know about KVM_EXIT_HYPERV_HCALL and complain about it or
>> even exit.  Just prevent anything bad from happening by handling
>> the hypercall in KVM and returning an "invalid hypercall" code.
> 
> I wonder if this has been encountered in real life or just found by code
> inspection?

Code inspection; I was looking for an excus^Wreason to submit your patch
for hypercall stubs during QEMU hard freeze, and it occurred to me that
you'd need the stubs even on older QEMU releases.  This patch avoids
that regression.

Paolo

>> Fixes: 83326e43f27e9a8a501427a0060f8af519a39bb2
>> Cc: Andrey Smetanin <asmetanin@virtuozzo.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/kvm/hyperv.c | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index b960d9ea171f..9e2becd20a59 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1237,14 +1237,17 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>>  		break;
>>  	case HVCALL_POST_MESSAGE:
>>  	case HVCALL_SIGNAL_EVENT:
>> -		vcpu->run->exit_reason = KVM_EXIT_HYPERV;
>> -		vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
>> -		vcpu->run->hyperv.u.hcall.input = param;
>> -		vcpu->run->hyperv.u.hcall.params[0] = ingpa;
>> -		vcpu->run->hyperv.u.hcall.params[1] = outgpa;
>> -		vcpu->arch.complete_userspace_io =
>> +		if (vcpu_to_synic(vcpu)->active) {
>> +			vcpu->run->exit_reason = KVM_EXIT_HYPERV;
>> +			vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
>> +			vcpu->run->hyperv.u.hcall.input = param;
>> +			vcpu->run->hyperv.u.hcall.params[0] = ingpa;
>> +			vcpu->run->hyperv.u.hcall.params[1] = outgpa;
>> +			vcpu->arch.complete_userspace_io =
>>  				kvm_hv_hypercall_complete_userspace;
>> -		return 0;
>> +			return 0;
>> +		}
>> +		/* fall through */
> 
> I'd rather put it the other way around, with explicit error path and a
> comment:
> 
> 
> 		/* don't bother userspace if it has no way to handle it */
> 		if (!vcpu_to_synic(vcpu)->active) {
> 			res = HV_STATUS_INVALID_HYPERCALL_CODE;
> 			break;
> 		}
> 
> but that's just nitpicking, so
> 
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Your version is better.

> [ Andrey has left the company so I'm not sure he even receives the
> messages sent to this address ]

He noticed and informed me.  I'll Cc both you and his gmail address in
the future.

Paolo