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

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]

Message-ID: <56FA5DDE.9030005@redhat.com>
Date:	Tue, 29 Mar 2016 12:50:06 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Roman Kagan <rkagan@...tuozzo.com>, linux-kernel@...r.kernel.org,
	kvm@...r.kernel.org, Andrey Smetanin <asmetanin@...tuozzo.com>
Subject: Re: [PATCH] KVM: Hyper-V: do not do hypercall userspace exits if
 SynIC is disabled



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@...tuozzo.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.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@...tuozzo.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