[PATCH 0/2] KVM: s390: two fixes for sthyi emulation (4.13)

[PATCH 0/2] KVM: s390: two fixes for sthyi emulation (4.13)

[Christian Borntraeger]

I plan to submit a pull request for these 2 patches soon. 

Heiko Carstens (2):
      KVM: s390: sthyi: fix sthyi inline assembly
      KVM: s390: sthyi: fix specification exception detection

 arch/s390/kvm/sthyi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Feedback welcome.

Christian

[PATCH 1/2] KVM: s390: sthyi: fix sthyi inline assembly

[Christian Borntraeger]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

The sthyi inline assembly misses register r3 within the clobber
list. The sthyi instruction will always write a return code to
register "R2+1", which in this case would be r3. Due to that we may
have register corruption and see host crashes or data corruption
depending on how gcc decided to allocate and use registers during
compile time.

Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
Cc: <stable@vger.kernel.org> # 4.8+
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sthyi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/sthyi.c b/arch/s390/kvm/sthyi.c
index 926b524..2773a2f 100644
--- a/arch/s390/kvm/sthyi.c
+++ b/arch/s390/kvm/sthyi.c
@@ -394,7 +394,7 @@ static int sthyi(u64 vaddr)
 		"srl     %[cc],28\n"
 		: [cc] "=d" (cc)
 		: [code] "d" (code), [addr] "a" (addr)
-		: "memory", "cc");
+		: "3", "memory", "cc");
 	return cc;
 }
 
-- 
2.7.4

[PATCH 2/2] KVM: s390: sthyi: fix specification exception detection

[Christian Borntraeger]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

sthyi should only generate a specification exception if the function
code is zero and the response buffer is not on a 4k boundary.

The current code would also test for unknown function codes if the
response buffer, that is currently only defined for function code 0,
is not on a 4k boundary and incorrectly inject a specification
exception instead of returning with condition code 3 and return code 4
(unsupported function code).

Fix this by moving the boundary check.

Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
Cc: <stable@vger.kernel.org> # 4.8+
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sthyi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/sthyi.c b/arch/s390/kvm/sthyi.c
index 2773a2f..a2e5c24 100644
--- a/arch/s390/kvm/sthyi.c
+++ b/arch/s390/kvm/sthyi.c
@@ -425,7 +425,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
 	VCPU_EVENT(vcpu, 3, "STHYI: fc: %llu addr: 0x%016llx", code, addr);
 	trace_kvm_s390_handle_sthyi(vcpu, code, addr);
 
-	if (reg1 == reg2 || reg1 & 1 || reg2 & 1 || addr & ~PAGE_MASK)
+	if (reg1 == reg2 || reg1 & 1 || reg2 & 1)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
 
 	if (code & 0xffff) {
@@ -433,6 +433,9 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
 		goto out;
 	}
 
+	if (addr & ~PAGE_MASK)
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+
 	/*
 	 * If the page has not yet been faulted in, we want to do that
 	 * now and not after all the expensive calculations.
-- 
2.7.4

Re: [PATCH 2/2] KVM: s390: sthyi: fix specification exception detection

[David Hildenbrand]

On 21.08.2017 14:27, Christian Borntraeger wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> sthyi should only generate a specification exception if the function
> code is zero and the response buffer is not on a 4k boundary.
> 
> The current code would also test for unknown function codes if the
> response buffer, that is currently only defined for function code 0,
> is not on a 4k boundary and incorrectly inject a specification
> exception instead of returning with condition code 3 and return code 4
> (unsupported function code).
> 
> Fix this by moving the boundary check.
> 
> Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
> Cc: <stable@vger.kernel.org> # 4.8+
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/sthyi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/sthyi.c b/arch/s390/kvm/sthyi.c
> index 2773a2f..a2e5c24 100644
> --- a/arch/s390/kvm/sthyi.c
> +++ b/arch/s390/kvm/sthyi.c
> @@ -425,7 +425,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
>  	VCPU_EVENT(vcpu, 3, "STHYI: fc: %llu addr: 0x%016llx", code, addr);
>  	trace_kvm_s390_handle_sthyi(vcpu, code, addr);
>  
> -	if (reg1 == reg2 || reg1 & 1 || reg2 & 1 || addr & ~PAGE_MASK)
> +	if (reg1 == reg2 || reg1 & 1 || reg2 & 1)
>  		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>  
>  	if (code & 0xffff) {
> @@ -433,6 +433,9 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
>  		goto out;
>  	}
>  
> +	if (addr & ~PAGE_MASK)
> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +
>  	/*
>  	 * If the page has not yet been faulted in, we want to do that
>  	 * now and not after all the expensive calculations.
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

Just wondering if this is really worth stable? (only function code 0 is
defined, so this should not happen in sane environments. or is this used
to test for support for new function codes (which would be strange but
possible)?)

-- 

Thanks,

David

Re: [PATCH 1/2] KVM: s390: sthyi: fix sthyi inline assembly

[David Hildenbrand]

On 21.08.2017 14:27, Christian Borntraeger wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> The sthyi inline assembly misses register r3 within the clobber
> list. The sthyi instruction will always write a return code to
> register "R2+1", which in this case would be r3. Due to that we may
> have register corruption and see host crashes or data corruption
> depending on how gcc decided to allocate and use registers during
> compile time.
> 
> Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
> Cc: <stable@vger.kernel.org> # 4.8+
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/sthyi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/sthyi.c b/arch/s390/kvm/sthyi.c
> index 926b524..2773a2f 100644
> --- a/arch/s390/kvm/sthyi.c
> +++ b/arch/s390/kvm/sthyi.c
> @@ -394,7 +394,7 @@ static int sthyi(u64 vaddr)
>  		"srl     %[cc],28\n"
>  		: [cc] "=d" (cc)
>  		: [code] "d" (code), [addr] "a" (addr)
> -		: "memory", "cc");
> +		: "3", "memory", "cc");
>  	return cc;
>  }
>  
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [PATCH 2/2] KVM: s390: sthyi: fix specification exception detection

[Christian Borntraeger]

On 08/21/2017 02:43 PM, David Hildenbrand wrote:
> On 21.08.2017 14:27, Christian Borntraeger wrote:
>> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>>
>> sthyi should only generate a specification exception if the function
>> code is zero and the response buffer is not on a 4k boundary.
>>
>> The current code would also test for unknown function codes if the
>> response buffer, that is currently only defined for function code 0,
>> is not on a 4k boundary and incorrectly inject a specification
>> exception instead of returning with condition code 3 and return code 4
>> (unsupported function code).
>>
>> Fix this by moving the boundary check.
>>
>> Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
>> Cc: <stable@vger.kernel.org> # 4.8+
>> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/kvm/sthyi.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/sthyi.c b/arch/s390/kvm/sthyi.c
>> index 2773a2f..a2e5c24 100644
>> --- a/arch/s390/kvm/sthyi.c
>> +++ b/arch/s390/kvm/sthyi.c
>> @@ -425,7 +425,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
>>  	VCPU_EVENT(vcpu, 3, "STHYI: fc: %llu addr: 0x%016llx", code, addr);
>>  	trace_kvm_s390_handle_sthyi(vcpu, code, addr);
>>  
>> -	if (reg1 == reg2 || reg1 & 1 || reg2 & 1 || addr & ~PAGE_MASK)
>> +	if (reg1 == reg2 || reg1 & 1 || reg2 & 1)
>>  		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>>  
>>  	if (code & 0xffff) {
>> @@ -433,6 +433,9 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
>>  		goto out;
>>  	}
>>  
>> +	if (addr & ~PAGE_MASK)
>> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> +
>>  	/*
>>  	 * If the page has not yet been faulted in, we want to do that
>>  	 * now and not after all the expensive calculations.
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> Just wondering if this is really worth stable? (only function code 0 is
> defined, so this should not happen in sane environments. or is this used
> to test for support for new function codes (which would be strange but
> possible)?)

I think it is a safety net if we ever implement new function codes. So yes
we want software being able to test for the new codes without having to
handle a program check. 

Re: [PATCH 1/2] KVM: s390: sthyi: fix sthyi inline assembly

[Cornelia Huck]

On Mon, 21 Aug 2017 14:27:44 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> The sthyi inline assembly misses register r3 within the clobber
> list. The sthyi instruction will always write a return code to
> register "R2+1", which in this case would be r3. Due to that we may
> have register corruption and see host crashes or data corruption
> depending on how gcc decided to allocate and use registers during
> compile time.
> 
> Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
> Cc: <stable@vger.kernel.org> # 4.8+
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/sthyi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/sthyi.c b/arch/s390/kvm/sthyi.c
> index 926b524..2773a2f 100644
> --- a/arch/s390/kvm/sthyi.c
> +++ b/arch/s390/kvm/sthyi.c
> @@ -394,7 +394,7 @@ static int sthyi(u64 vaddr)
>  		"srl     %[cc],28\n"
>  		: [cc] "=d" (cc)
>  		: [code] "d" (code), [addr] "a" (addr)
> -		: "memory", "cc");
> +		: "3", "memory", "cc");
>  	return cc;
>  }
>  

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 2/2] KVM: s390: sthyi: fix specification exception detection

[Cornelia Huck]

On Mon, 21 Aug 2017 14:27:45 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> sthyi should only generate a specification exception if the function
> code is zero and the response buffer is not on a 4k boundary.
> 
> The current code would also test for unknown function codes if the
> response buffer, that is currently only defined for function code 0,
> is not on a 4k boundary and incorrectly inject a specification
> exception instead of returning with condition code 3 and return code 4
> (unsupported function code).
> 
> Fix this by moving the boundary check.
> 
> Fixes: 95ca2cb57985 ("KVM: s390: Add sthyi emulation")
> Cc: <stable@vger.kernel.org> # 4.8+
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/sthyi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>