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

2019-09-04 Thread Thomas Huth 
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 
 Reviewed-by: Christian Borntraeger 
 Reviewed-by: Cornelia Huck 
 Signed-off-by: Thomas Huth 
 ---
  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

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

2019-09-04 Thread David Hildenbrand 
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 
>>> Reviewed-by: Christian Borntraeger 
>>> Reviewed-by: Cornelia Huck 
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>  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.

-- 

Thanks,

David / dhildenb

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

2019-09-04 Thread David Hildenbrand 
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 
>>> Reviewed-by: Christian Borntraeger 
>>> Reviewed-by: Cornelia Huck 
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>  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
> 


-- 

Thanks,

David / dhildenb

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

2019-09-04 Thread Christian Borntraeger 



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 
>> Reviewed-by: Christian Borntraeger 
>> Reviewed-by: Cornelia Huck 
>> Signed-off-by: Thomas Huth 
>> ---
>>  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 49d779ae..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 
>

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

2019-09-04 Thread Thomas Huth 
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 
>> Reviewed-by: Christian Borntraeger 
>> Reviewed-by: Cornelia Huck 
>> Signed-off-by: Thomas Huth 
>> ---
>>  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

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

2019-09-04 Thread David Hildenbrand 
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 
> Reviewed-by: Christian Borntraeger 
> Reviewed-by: Cornelia Huck 
> Signed-off-by: Thomas Huth 
> ---
>  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 49d779ae..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 

-- 

Thanks,

David / dhildenb

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

2019-09-04 Thread Thomas Huth 
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 
Reviewed-by: Christian Borntraeger 
Reviewed-by: Cornelia Huck 
Signed-off-by: Thomas Huth 
---
 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)
+
 /* 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 49d779ae..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)) {
-- 
2.18.1