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

[Laurent Dufour]

Le 20/03/2020 Ã 13:22, Greg Kurz a ÃcritÂ:
> 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) ?

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@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.

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: