diff mbox series

[v2,1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs

Message ID 20190904085200.29021-2-thuth@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs | expand

Commit Message

Thomas Huth Sept. 4, 2019, 8:51 a.m. UTC 
If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
clearly indicates that something went wrong in the KVM userspace
application. The x86 variant of KVM already contains a check for
bad bits, so let's do the same on s390x now, too.

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/s390/include/uapi/asm/kvm.h | 6 ++++++
 arch/s390/kvm/kvm-s390.c         | 4 ++++
 2 files changed, 10 insertions(+)

Comments

David Hildenbrand Sept. 4, 2019, 9:05 a.m. UTC | #1 
On 04.09.19 10:51, Thomas Huth wrote:
> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
> clearly indicates that something went wrong in the KVM userspace
> application. The x86 variant of KVM already contains a check for
> bad bits, so let's do the same on s390x now, too.
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  arch/s390/include/uapi/asm/kvm.h | 6 ++++++
>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 47104e5b47fd..436ec7636927 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>  #define KVM_SYNC_GSCB   (1UL << 9)
>  #define KVM_SYNC_BPBC   (1UL << 10)
>  #define KVM_SYNC_ETOKEN (1UL << 11)
> +
> +#define KVM_SYNC_S390_VALID_FIELDS \
> +	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
> +	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
> +

We didn't care about the S390 for the actual flags, why care now?


>  /* length and alignment of the sdnx as a power of two */
>  #define SDNXC 8
>  #define SDNXL (1UL << SDNXC)
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 49d7722229ae..a7d7dedfe527 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3998,6 +3998,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	if (kvm_run->immediate_exit)
>  		return -EINTR;
>  
> +	if (kvm_run->kvm_valid_regs & ~KVM_SYNC_S390_VALID_FIELDS ||
> +	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
> +		return -EINVAL;
> +
>  	vcpu_load(vcpu);
>  
>  	if (guestdbg_exit_pending(vcpu)) {
> 

Reviewed-by: David Hildenbrand <david@redhat.com>
Thomas Huth Sept. 4, 2019, 9:10 a.m. UTC | #2 
On 04/09/2019 11.05, David Hildenbrand wrote:
> On 04.09.19 10:51, Thomas Huth wrote:
>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>> clearly indicates that something went wrong in the KVM userspace
>> application. The x86 variant of KVM already contains a check for
>> bad bits, so let's do the same on s390x now, too.
>>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  arch/s390/include/uapi/asm/kvm.h | 6 ++++++
>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 47104e5b47fd..436ec7636927 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>> +
>> +#define KVM_SYNC_S390_VALID_FIELDS \
>> +	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>> +	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>> +
> 
> We didn't care about the S390 for the actual flags, why care now?

x86 does the same, and we don't want to be worse than x86, do we? ;-)

Honestly, this was just one of the differences that I noticed while
porting the sync_regs_test from x86 to s390x.

 Thomas
Christian Borntraeger Sept. 4, 2019, 9:11 a.m. UTC | #3 
On 04.09.19 11:05, David Hildenbrand wrote:
> On 04.09.19 10:51, Thomas Huth wrote:
>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>> clearly indicates that something went wrong in the KVM userspace
>> application. The x86 variant of KVM already contains a check for
>> bad bits, so let's do the same on s390x now, too.
>>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  arch/s390/include/uapi/asm/kvm.h | 6 ++++++
>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 47104e5b47fd..436ec7636927 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>> +
>> +#define KVM_SYNC_S390_VALID_FIELDS \
>> +	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>> +	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>> +
> 
> We didn't care about the S390 for the actual flags, why care now?

I think it makes sense to have the interface as defined as possible. If for some
reason userspace sets a wrong bit this would be undetected. If we at a later point
in time use that bit this would resultin strange problems.
> 
> 
>>  /* length and alignment of the sdnx as a power of two */
>>  #define SDNXC 8
>>  #define SDNXL (1UL << SDNXC)
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 49d7722229ae..a7d7dedfe527 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -3998,6 +3998,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  	if (kvm_run->immediate_exit)
>>  		return -EINTR;
>>  
>> +	if (kvm_run->kvm_valid_regs & ~KVM_SYNC_S390_VALID_FIELDS ||
>> +	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
>> +		return -EINVAL;
>> +
>>  	vcpu_load(vcpu);
>>  
>>  	if (guestdbg_exit_pending(vcpu)) {
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
David Hildenbrand Sept. 4, 2019, 9:11 a.m. UTC | #4 
On 04.09.19 11:10, Thomas Huth wrote:
> On 04/09/2019 11.05, David Hildenbrand wrote:
>> On 04.09.19 10:51, Thomas Huth wrote:
>>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>>> clearly indicates that something went wrong in the KVM userspace
>>> application. The x86 variant of KVM already contains a check for
>>> bad bits, so let's do the same on s390x now, too.
>>>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  arch/s390/include/uapi/asm/kvm.h | 6 ++++++
>>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>> index 47104e5b47fd..436ec7636927 100644
>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>>> +
>>> +#define KVM_SYNC_S390_VALID_FIELDS \
>>> +	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>>> +	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>>> +
>>
>> We didn't care about the S390 for the actual flags, why care now?
> 
> x86 does the same, and we don't want to be worse than x86, do we? ;-)

Yeah, but they do have X86 in every flag - in contrast to us.

> 
> Honestly, this was just one of the differences that I noticed while
> porting the sync_regs_test from x86 to s390x.
> 
>  Thomas
>
David Hildenbrand Sept. 4, 2019, 9:15 a.m. UTC | #5 
On 04.09.19 11:11, Christian Borntraeger wrote:
> 
> 
> On 04.09.19 11:05, David Hildenbrand wrote:
>> On 04.09.19 10:51, Thomas Huth wrote:
>>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>>> clearly indicates that something went wrong in the KVM userspace
>>> application. The x86 variant of KVM already contains a check for
>>> bad bits, so let's do the same on s390x now, too.
>>>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  arch/s390/include/uapi/asm/kvm.h | 6 ++++++
>>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>> index 47104e5b47fd..436ec7636927 100644
>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>>> +
>>> +#define KVM_SYNC_S390_VALID_FIELDS \
>>> +	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>>> +	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>>> +
>>
>> We didn't care about the S390 for the actual flags, why care now?
> 
> I think it makes sense to have the interface as defined as possible. If for some
> reason userspace sets a wrong bit this would be undetected. If we at a later point
> in time use that bit this would resultin strange problems.

Not arguing about the concept of checking for valid bits. Was just
wondering if the "S390" part in the name makes sense at all. But you
guys seem to have a consent here.
Thomas Huth Sept. 4, 2019, 9:21 a.m. UTC | #6 
On 04/09/2019 11.15, David Hildenbrand wrote:
> On 04.09.19 11:11, Christian Borntraeger wrote:
>>
>>
>> On 04.09.19 11:05, David Hildenbrand wrote:
>>> On 04.09.19 10:51, Thomas Huth wrote:
>>>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>>>> clearly indicates that something went wrong in the KVM userspace
>>>> application. The x86 variant of KVM already contains a check for
>>>> bad bits, so let's do the same on s390x now, too.
>>>>
>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  arch/s390/include/uapi/asm/kvm.h | 6 ++++++
>>>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>>>> index 47104e5b47fd..436ec7636927 100644
>>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>>>> +
>>>> +#define KVM_SYNC_S390_VALID_FIELDS \
>>>> +	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>>>> +	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>>>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>>>> +
>>>
>>> We didn't care about the S390 for the actual flags, why care now?
>>
>> I think it makes sense to have the interface as defined as possible. If for some
>> reason userspace sets a wrong bit this would be undetected. If we at a later point
>> in time use that bit this would resultin strange problems.
> 
> Not arguing about the concept of checking for valid bits. Was just
> wondering if the "S390" part in the name makes sense at all. But you
> guys seem to have a consent here.

Oh, I guess we both got your question wrong... Well, I don't care too
much whether we've got an "S390" in there or not ... so unless there is
a real good reason to remove it, let's simply keep it this way.

 Thomas
diff mbox series

Patch

diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 47104e5b47fd..436ec7636927 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -231,6 +231,12 @@  struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+
+#define KVM_SYNC_S390_VALID_FIELDS \
+	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
+	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
+	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
 #define SDNXL (1UL << SDNXC)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 49d7722229ae..a7d7dedfe527 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3998,6 +3998,10 @@  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	if (kvm_run->immediate_exit)
 		return -EINTR;
 
+	if (kvm_run->kvm_valid_regs & ~KVM_SYNC_S390_VALID_FIELDS ||
+	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
+		return -EINVAL;
+
 	vcpu_load(vcpu);
 
 	if (guestdbg_exit_pending(vcpu)) {