lkml.org 
[lkml]   [2020]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls
From
Date
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:
>

\
 
 \ /
  Last update: 2020-03-20 15:45    [W:0.083 / U:0.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site