On Fri, 20 Mar 2020 11:26:42 +0100 Laurent Dufour <ldufour@xxxxxxxxxxxxx> 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) ? Shouldn't we also check the HV bit of the calling MSR as well to disambiguate SVM and UV ? > > Cc: Bharata B Rao <bharata@xxxxxxxxxxxxx> > Cc: Paul Mackerras <paulus@xxxxxxxxxx> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx> > Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxx> > --- > 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. > 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:
