[1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls
diff mbox series

Message ID 20200320102643.15516-2-ldufour@linux.ibm.com
State Accepted
Headers show
Series
  • Fix SVM hang at startup
Related show

Commit Message

Laurent Dufour March 20, 2020, 10:26 a.m. UTC
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.

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(-)

Comments

Greg Kurz March 20, 2020, 12:22 p.m. UTC | #1
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) ?

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

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

>  		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:
Laurent Dufour March 20, 2020, 2:43 p.m. UTC | #2
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:
>
Ram Pai March 21, 2020, 12:40 a.m. UTC | #3
On Fri, Mar 20, 2020 at 11:26:42AM +0100, Laurent Dufour 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.
> 
> 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>

Reviewed-by: Ram Pai <linuxram@us.ibnm.com>

> ---
>  arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
Paul Mackerras March 23, 2020, 11:43 p.m. UTC | #4
On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote:
> 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) ?
> 
> Shouldn't we also check the HV bit of the calling MSR as well to
> disambiguate SVM and UV ?

The trouble with doing that (checking the HV bit) is that KVM does not
expect to see the HV bit set on an interrupt that occurred while we
were in the guest, and if it is set, it indicates a serious problem,
i.e. that an interrupt occurred while we were in the code that
transitions from host context to guest context, or from guest context
to host context.  In those cases we don't know how much of the
transition has been completed and therefore whether we have guest
values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs).
If we do see HV set then KVM reports a severe error to userspace which
should cause userspace to terminate the guest.

Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1
when transitioning to KVM.

Paul.
Greg Kurz March 24, 2020, noon UTC | #5
On Tue, 24 Mar 2020 10:43:23 +1100
Paul Mackerras <paulus@ozlabs.org> wrote:

> On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote:
> > 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) ?
> > 
> > Shouldn't we also check the HV bit of the calling MSR as well to
> > disambiguate SVM and UV ?
> 
> The trouble with doing that (checking the HV bit) is that KVM does not
> expect to see the HV bit set on an interrupt that occurred while we
> were in the guest, and if it is set, it indicates a serious problem,
> i.e. that an interrupt occurred while we were in the code that
> transitions from host context to guest context, or from guest context
> to host context.  In those cases we don't know how much of the
> transition has been completed and therefore whether we have guest
> values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs).
> If we do see HV set then KVM reports a severe error to userspace which
> should cause userspace to terminate the guest.
> 
> Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1
> when transitioning to KVM.
> 

Indeed... thanks for the clarification. So I guess we'll just assume
that the UV doesn't reflect these SVM specific hcalls if they happened
to be issued by the guest then.

Cheers,

--
Greg

> Paul.
Laurent Dufour March 24, 2020, 1:13 p.m. UTC | #6
Le 24/03/2020 à 13:00, Greg Kurz a écrit :
> On Tue, 24 Mar 2020 10:43:23 +1100
> Paul Mackerras <paulus@ozlabs.org> wrote:
> 
>> On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote:
>>> 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) ?
>>>
>>> Shouldn't we also check the HV bit of the calling MSR as well to
>>> disambiguate SVM and UV ?
>>
>> The trouble with doing that (checking the HV bit) is that KVM does not
>> expect to see the HV bit set on an interrupt that occurred while we
>> were in the guest, and if it is set, it indicates a serious problem,
>> i.e. that an interrupt occurred while we were in the code that
>> transitions from host context to guest context, or from guest context
>> to host context.  In those cases we don't know how much of the
>> transition has been completed and therefore whether we have guest
>> values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs).
>> If we do see HV set then KVM reports a severe error to userspace which
>> should cause userspace to terminate the guest.
>>
>> Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1
>> when transitioning to KVM.
>>
> 
> Indeed... thanks for the clarification. So I guess we'll just assume
> that the UV doesn't reflect these SVM specific hcalls if they happened
> to be issued by the guest then.

As mentioned in the series's description:
"It is assumed that the UV will filtered out such Hcalls made by a malicious
SVM."

> Cheers,
> 
> --
> Greg
> 
>> Paul.
>

Patch
diff mbox series

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));
 		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: