Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls

[Laurent Dufour <ldufour@linux.ibm.com>]

Le 20/03/2020 à 13:22, Greg Kurz a écrit :
> On Fri, 20 Mar 2020 11:26:42 +0100
> Laurent Dufour <ldufour@linux.ibm.com> wrote:
> 
>> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing
>> prevent a malicious VM or SVM to call them. This could lead to weird result
>> and should be filtered out.
>>
>> Checking the Secure bit of the calling MSR ensure that the call is coming
>> from either the Ultravisor or a SVM. But any system call made from a SVM
>> are going through the Ultravisor, and the Ultravisor should filter out
>> these malicious call. This way, only the Ultravisor is able to make such a
>> Hcall.
> 
> "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ?

If it doesn't, a malicious SVM would be able to call UV reserved Hcall like 
H_SVM_INIT_ABORT, etc... which is not a good idea.

> 
> Shouldn't we also check the HV bit of the calling MSR as well to
> disambiguate SVM and UV ?

That's another way to do so, but since the SVM Hcall are going through the UV, 
it seems the right place (the UV) to do the filtering.

>>
>> Cc: Bharata B Rao <bharata@linux.ibm.com>
>> Cc: Paul Mackerras <paulus@ozlabs.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++-----------
>>   1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 33be4d93248a..43773182a737 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
>>   					 kvmppc_get_gpr(vcpu, 6));
>>   		break;
>>   	case H_SVM_PAGE_IN:
>> -		ret = kvmppc_h_svm_page_in(vcpu->kvm,
>> -					   kvmppc_get_gpr(vcpu, 4),
>> -					   kvmppc_get_gpr(vcpu, 5),
>> -					   kvmppc_get_gpr(vcpu, 6));
>> +		ret = H_UNSUPPORTED;
>> +		if (kvmppc_get_srr1(vcpu) & MSR_S)
>> +			ret = kvmppc_h_svm_page_in(vcpu->kvm,
>> +						   kvmppc_get_gpr(vcpu, 4),
>> +						   kvmppc_get_gpr(vcpu, 5),
>> +						   kvmppc_get_gpr(vcpu, 6));
> 
> If calling kvmppc_h_svm_page_in() produces a "weird result" when
> the MSR_S bit isn't set, then I think it should do the checking
> itself, ie. pass vcpu.
> 
> This would also prevent adding that many lines in kvmppc_pseries_do_hcall()
> which is a big enough function already. The checking could be done in a
> helper in book3s_hv_uvmem.c and used by all UV specific hcalls.

I'm not convinced that would be better, and I followed the way checks for other 
Hcalls has been made (see H_TLB_INVALIDATE,..).

I agree  kvmppc_pseries_do_hcall() is long but this is just a big switch(), 
quite linear.

> 
>>   		break;
>>   	case H_SVM_PAGE_OUT:
>> -		ret = kvmppc_h_svm_page_out(vcpu->kvm,
>> -					    kvmppc_get_gpr(vcpu, 4),
>> -					    kvmppc_get_gpr(vcpu, 5),
>> -					    kvmppc_get_gpr(vcpu, 6));
>> +		ret = H_UNSUPPORTED;
>> +		if (kvmppc_get_srr1(vcpu) & MSR_S)
>> +			ret = kvmppc_h_svm_page_out(vcpu->kvm,
>> +						    kvmppc_get_gpr(vcpu, 4),
>> +						    kvmppc_get_gpr(vcpu, 5),
>> +						    kvmppc_get_gpr(vcpu, 6));
>>   		break;
>>   	case H_SVM_INIT_START:
>> -		ret = kvmppc_h_svm_init_start(vcpu->kvm);
>> +		ret = H_UNSUPPORTED;
>> +		if (kvmppc_get_srr1(vcpu) & MSR_S)
>> +			ret = kvmppc_h_svm_init_start(vcpu->kvm);
>>   		break;
>>   	case H_SVM_INIT_DONE:
>> -		ret = kvmppc_h_svm_init_done(vcpu->kvm);
>> +		ret = H_UNSUPPORTED;
>> +		if (kvmppc_get_srr1(vcpu) & MSR_S)
>> +			ret = kvmppc_h_svm_init_done(vcpu->kvm);
>>   		break;
>>   	case H_SVM_INIT_ABORT:
>> -		ret = kvmppc_h_svm_init_abort(vcpu->kvm);
>> +		ret = H_UNSUPPORTED;
>> +		if (kvmppc_get_srr1(vcpu) & MSR_S)
>> +			ret = kvmppc_h_svm_init_abort(vcpu->kvm);
>>   		break;
>>   
>>   	default:
>