diff mbox series

[RFC,23/37] KVM: s390: protvirt: Make sure prefix is always protected

Message ID 20191024114059.102802-24-frankja@linux.ibm.com
State New, archived
Headers show
Series KVM: s390: Add support for protected VMs | expand

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Cornelia Huck Nov. 18, 2019, 4:39 p.m. UTC | #1
On Thu, 24 Oct 2019 07:40:45 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

Add at least a short sentence here?

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index eddc9508c1b1..17a78774c617 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>  		rc = gmap_mprotect_notify(vcpu->arch.gmap,
>  					  kvm_s390_get_prefix(vcpu),
>  					  PAGE_SIZE * 2, PROT_WRITE);
> +		if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) {
> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
> +						  kvm_s390_get_prefix(vcpu));
> +			WARN_ON_ONCE(rc && rc != -EEXIST);
> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
> +						  kvm_s390_get_prefix(vcpu) + PAGE_SIZE);
> +			WARN_ON_ONCE(rc && rc != -EEXIST);
> +			rc = 0;

So, what happens if we have an error other than -EEXIST (which
presumably means that we already protected it) here? The page is not
protected, and further accesses will get an error? (Another question:
what can actually go wrong here?)

> +		}
>  		if (rc) {
>  			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
>  			return rc;
Janosch Frank Nov. 19, 2019, 8:11 a.m. UTC | #2
On 11/18/19 5:39 PM, Cornelia Huck wrote:
> On Thu, 24 Oct 2019 07:40:45 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
> Add at least a short sentence here?

For protected VMs the lowcore does not only need to be mapped, but also
needs to be protected memory, if not we'll get a validity interception
when trying to run it.

> 
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index eddc9508c1b1..17a78774c617 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>>  		rc = gmap_mprotect_notify(vcpu->arch.gmap,
>>  					  kvm_s390_get_prefix(vcpu),
>>  					  PAGE_SIZE * 2, PROT_WRITE);
>> +		if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) {
>> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
>> +						  kvm_s390_get_prefix(vcpu));
>> +			WARN_ON_ONCE(rc && rc != -EEXIST);
>> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
>> +						  kvm_s390_get_prefix(vcpu) + PAGE_SIZE);
>> +			WARN_ON_ONCE(rc && rc != -EEXIST);
>> +			rc = 0;
> 
> So, what happens if we have an error other than -EEXIST (which
> presumably means that we already protected it) here? The page is not
> protected, and further accesses will get an error? (Another question:
> what can actually go wrong here?)

If KVM or QEMU write into a lowcore, we will fail the integrity check on
import and this cpu will never run again.

In retrospect a warn_on_once might be the wrong error handling here.

> 
>> +		}
>>  		if (rc) {
>>  			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
>>  			return rc;
>
Cornelia Huck Nov. 19, 2019, 9:45 a.m. UTC | #3
On Tue, 19 Nov 2019 09:11:11 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/18/19 5:39 PM, Cornelia Huck wrote:
> > On Thu, 24 Oct 2019 07:40:45 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> > 
> > Add at least a short sentence here?  
> 
> For protected VMs the lowcore does not only need to be mapped, but also
> needs to be protected memory, if not we'll get a validity interception
> when trying to run it.

Much better, thanks!

> 
> >   
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>  arch/s390/kvm/kvm-s390.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >> index eddc9508c1b1..17a78774c617 100644
> >> --- a/arch/s390/kvm/kvm-s390.c
> >> +++ b/arch/s390/kvm/kvm-s390.c
> >> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
> >>  		rc = gmap_mprotect_notify(vcpu->arch.gmap,
> >>  					  kvm_s390_get_prefix(vcpu),
> >>  					  PAGE_SIZE * 2, PROT_WRITE);
> >> +		if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) {
> >> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
> >> +						  kvm_s390_get_prefix(vcpu));
> >> +			WARN_ON_ONCE(rc && rc != -EEXIST);
> >> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
> >> +						  kvm_s390_get_prefix(vcpu) + PAGE_SIZE);
> >> +			WARN_ON_ONCE(rc && rc != -EEXIST);
> >> +			rc = 0;  
> > 
> > So, what happens if we have an error other than -EEXIST (which
> > presumably means that we already protected it) here? The page is not
> > protected, and further accesses will get an error? (Another question:
> > what can actually go wrong here?)  
> 
> If KVM or QEMU write into a lowcore, we will fail the integrity check on
> import and this cpu will never run again.

From the guest's POV, is that similar to a cpu going into xstop?
 
> In retrospect a warn_on_once might be the wrong error handling here.
> 
> >   
> >> +		}
> >>  		if (rc) {
> >>  			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
> >>  			return rc;  
> >   
> 
>
Janosch Frank Nov. 19, 2019, 10:08 a.m. UTC | #4
On 11/19/19 10:45 AM, Cornelia Huck wrote:
> On Tue, 19 Nov 2019 09:11:11 +0100
[...]
>>>
>>> So, what happens if we have an error other than -EEXIST (which
>>> presumably means that we already protected it) here? The page is not
>>> protected, and further accesses will get an error? (Another question:
>>> what can actually go wrong here?)  
>>
>> If KVM or QEMU write into a lowcore, we will fail the integrity check on
>> import and this cpu will never run again.
> 
> From the guest's POV, is that similar to a cpu going into xstop?

Not really, I'm wondering what happens, if we try to send a restart to
such a cpu.

>  
>> In retrospect a warn_on_once might be the wrong error handling here.
>>
>>>   
>>>> +		}
>>>>  		if (rc) {
>>>>  			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
>>>>  			return rc;  
>>>   
>>
>>
>
David Hildenbrand Nov. 19, 2019, 10:18 a.m. UTC | #5
On 24.10.19 13:40, Janosch Frank wrote:
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index eddc9508c1b1..17a78774c617 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>   		rc = gmap_mprotect_notify(vcpu->arch.gmap,
>   					  kvm_s390_get_prefix(vcpu),
>   					  PAGE_SIZE * 2, PROT_WRITE);
> +		if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) {
> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
> +						  kvm_s390_get_prefix(vcpu));
> +			WARN_ON_ONCE(rc && rc != -EEXIST);
> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
> +						  kvm_s390_get_prefix(vcpu) + PAGE_SIZE);
> +			WARN_ON_ONCE(rc && rc != -EEXIST);
> +			rc = 0;
> +		}

... what if userspace reads the prefix pages just after these calls? 
validity? :/

>   		if (rc) {
>   			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
>   			return rc;
>
Janosch Frank Nov. 19, 2019, 11:36 a.m. UTC | #6
On 11/19/19 11:18 AM, David Hildenbrand wrote:
> On 24.10.19 13:40, Janosch Frank wrote:
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   arch/s390/kvm/kvm-s390.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index eddc9508c1b1..17a78774c617 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>>   		rc = gmap_mprotect_notify(vcpu->arch.gmap,
>>   					  kvm_s390_get_prefix(vcpu),
>>   					  PAGE_SIZE * 2, PROT_WRITE);
>> +		if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) {
>> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
>> +						  kvm_s390_get_prefix(vcpu));
>> +			WARN_ON_ONCE(rc && rc != -EEXIST);
>> +			rc = uv_convert_to_secure(vcpu->arch.gmap,
>> +						  kvm_s390_get_prefix(vcpu) + PAGE_SIZE);
>> +			WARN_ON_ONCE(rc && rc != -EEXIST);
>> +			rc = 0;
>> +		}
> 
> ... what if userspace reads the prefix pages just after these calls? 
> validity? :/

Currently yes, we're working with firmware to fix this.

> 
>>   		if (rc) {
>>   			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
>>   			return rc;
>>
>
diff mbox series

Patch

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index eddc9508c1b1..17a78774c617 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3646,6 +3646,15 @@  static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
 		rc = gmap_mprotect_notify(vcpu->arch.gmap,
 					  kvm_s390_get_prefix(vcpu),
 					  PAGE_SIZE * 2, PROT_WRITE);
+		if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) {
+			rc = uv_convert_to_secure(vcpu->arch.gmap,
+						  kvm_s390_get_prefix(vcpu));
+			WARN_ON_ONCE(rc && rc != -EEXIST);
+			rc = uv_convert_to_secure(vcpu->arch.gmap,
+						  kvm_s390_get_prefix(vcpu) + PAGE_SIZE);
+			WARN_ON_ONCE(rc && rc != -EEXIST);
+			rc = 0;
+		}
 		if (rc) {
 			kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu);
 			return rc;