* [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs
@ 2019-09-04 8:51 Thomas Huth
2019-09-04 8:51 ` [PATCH v2 1/2] KVM: s390: Disallow " Thomas Huth
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Thomas Huth @ 2019-09-04 8:51 UTC (permalink / raw)
To: kvm, Christian Borntraeger, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, linux-s390, linux-kernel
Avoid invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x.
v2:
- Split the single patch from v1 into two separate patches
(I've kept the Reviewed-bys from v1, but if you don't agree with the
patch description of the 2nd patch, please complain)
Thomas Huth (2):
KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs
on s390x
arch/s390/include/uapi/asm/kvm.h | 6 ++++
arch/s390/kvm/kvm-s390.c | 4 +++
.../selftests/kvm/s390x/sync_regs_test.c | 30 +++++++++++++++++++
3 files changed, 40 insertions(+)
--
2.18.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 8:51 [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Thomas Huth
@ 2019-09-04 8:51 ` Thomas Huth
2019-09-04 9:05 ` David Hildenbrand
2019-09-04 8:52 ` [PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x Thomas Huth
2019-09-05 8:00 ` [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Janosch Frank
2 siblings, 1 reply; 11+ messages in thread
From: Thomas Huth @ 2019-09-04 8:51 UTC (permalink / raw)
To: kvm, Christian Borntraeger, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, linux-s390, linux-kernel
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)
+
/* 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)) {
--
2.18.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x
2019-09-04 8:51 [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Thomas Huth
2019-09-04 8:51 ` [PATCH v2 1/2] KVM: s390: Disallow " Thomas Huth
@ 2019-09-04 8:52 ` Thomas Huth
2019-09-04 9:06 ` David Hildenbrand
2019-09-05 8:00 ` [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Janosch Frank
2 siblings, 1 reply; 11+ messages in thread
From: Thomas Huth @ 2019-09-04 8:52 UTC (permalink / raw)
To: kvm, Christian Borntraeger, Janosch Frank
Cc: David Hildenbrand, Cornelia Huck, linux-s390, linux-kernel
Now that we disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
on s390x, too, we should also check this condition in the selftests.
The code has been taken from the x86-version of the sync_regs_test.
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>
---
.../selftests/kvm/s390x/sync_regs_test.c | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index bbc93094519b..d5290b4ad636 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -85,6 +85,36 @@ int main(int argc, char *argv[])
run = vcpu_state(vm, VCPU_ID);
+ /* Request reading invalid register set from VCPU. */
+ run->kvm_valid_regs = INVALID_SYNC_FIELD;
+ rv = _vcpu_run(vm, VCPU_ID);
+ TEST_ASSERT(rv < 0 && errno == EINVAL,
+ "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
+ rv);
+ vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+
+ run->kvm_valid_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
+ rv = _vcpu_run(vm, VCPU_ID);
+ TEST_ASSERT(rv < 0 && errno == EINVAL,
+ "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
+ rv);
+ vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+
+ /* Request setting invalid register set into VCPU. */
+ run->kvm_dirty_regs = INVALID_SYNC_FIELD;
+ rv = _vcpu_run(vm, VCPU_ID);
+ TEST_ASSERT(rv < 0 && errno == EINVAL,
+ "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
+ rv);
+ vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+
+ run->kvm_dirty_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
+ rv = _vcpu_run(vm, VCPU_ID);
+ TEST_ASSERT(rv < 0 && errno == EINVAL,
+ "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
+ rv);
+ vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+
/* Request and verify all valid register sets. */
run->kvm_valid_regs = TEST_SYNC_FIELDS;
rv = _vcpu_run(vm, VCPU_ID);
--
2.18.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 8:51 ` [PATCH v2 1/2] KVM: s390: Disallow " Thomas Huth
@ 2019-09-04 9:05 ` David Hildenbrand
2019-09-04 9:10 ` Thomas Huth
2019-09-04 9:11 ` Christian Borntraeger
0 siblings, 2 replies; 11+ messages in thread
From: David Hildenbrand @ 2019-09-04 9:05 UTC (permalink / raw)
To: Thomas Huth, kvm, Christian Borntraeger, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
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>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x
2019-09-04 8:52 ` [PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x Thomas Huth
@ 2019-09-04 9:06 ` David Hildenbrand
0 siblings, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2019-09-04 9:06 UTC (permalink / raw)
To: Thomas Huth, kvm, Christian Borntraeger, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
On 04.09.19 10:52, Thomas Huth wrote:
> Now that we disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
> on s390x, too, we should also check this condition in the selftests.
> The code has been taken from the x86-version of the sync_regs_test.
>
> 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>
> ---
> .../selftests/kvm/s390x/sync_regs_test.c | 30 +++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> index bbc93094519b..d5290b4ad636 100644
> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> @@ -85,6 +85,36 @@ int main(int argc, char *argv[])
>
> run = vcpu_state(vm, VCPU_ID);
>
> + /* Request reading invalid register set from VCPU. */
> + run->kvm_valid_regs = INVALID_SYNC_FIELD;
> + rv = _vcpu_run(vm, VCPU_ID);
> + TEST_ASSERT(rv < 0 && errno == EINVAL,
> + "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
> + rv);
> + vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
> +
> + run->kvm_valid_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
> + rv = _vcpu_run(vm, VCPU_ID);
> + TEST_ASSERT(rv < 0 && errno == EINVAL,
> + "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
> + rv);
> + vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
> +
> + /* Request setting invalid register set into VCPU. */
> + run->kvm_dirty_regs = INVALID_SYNC_FIELD;
> + rv = _vcpu_run(vm, VCPU_ID);
> + TEST_ASSERT(rv < 0 && errno == EINVAL,
> + "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
> + rv);
> + vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
> +
> + run->kvm_dirty_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
> + rv = _vcpu_run(vm, VCPU_ID);
> + TEST_ASSERT(rv < 0 && errno == EINVAL,
> + "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
> + rv);
> + vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
> +
> /* Request and verify all valid register sets. */
> run->kvm_valid_regs = TEST_SYNC_FIELDS;
> rv = _vcpu_run(vm, VCPU_ID);
>
Looks sane to me.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 9:05 ` David Hildenbrand
@ 2019-09-04 9:10 ` Thomas Huth
2019-09-04 9:11 ` David Hildenbrand
2019-09-04 9:11 ` Christian Borntraeger
1 sibling, 1 reply; 11+ messages in thread
From: Thomas Huth @ 2019-09-04 9:10 UTC (permalink / raw)
To: David Hildenbrand, kvm, Christian Borntraeger, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 9:05 ` David Hildenbrand
2019-09-04 9:10 ` Thomas Huth
@ 2019-09-04 9:11 ` Christian Borntraeger
2019-09-04 9:15 ` David Hildenbrand
1 sibling, 1 reply; 11+ messages in thread
From: Christian Borntraeger @ 2019-09-04 9:11 UTC (permalink / raw)
To: David Hildenbrand, Thomas Huth, kvm, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
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>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 9:10 ` Thomas Huth
@ 2019-09-04 9:11 ` David Hildenbrand
0 siblings, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2019-09-04 9:11 UTC (permalink / raw)
To: Thomas Huth, kvm, Christian Borntraeger, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
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
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 9:11 ` Christian Borntraeger
@ 2019-09-04 9:15 ` David Hildenbrand
2019-09-04 9:21 ` Thomas Huth
0 siblings, 1 reply; 11+ messages in thread
From: David Hildenbrand @ 2019-09-04 9:15 UTC (permalink / raw)
To: Christian Borntraeger, Thomas Huth, kvm, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
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.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 9:15 ` David Hildenbrand
@ 2019-09-04 9:21 ` Thomas Huth
0 siblings, 0 replies; 11+ messages in thread
From: Thomas Huth @ 2019-09-04 9:21 UTC (permalink / raw)
To: David Hildenbrand, Christian Borntraeger, kvm, Janosch Frank
Cc: Cornelia Huck, linux-s390, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs
2019-09-04 8:51 [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Thomas Huth
2019-09-04 8:51 ` [PATCH v2 1/2] KVM: s390: Disallow " Thomas Huth
2019-09-04 8:52 ` [PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x Thomas Huth
@ 2019-09-05 8:00 ` Janosch Frank
2 siblings, 0 replies; 11+ messages in thread
From: Janosch Frank @ 2019-09-05 8:00 UTC (permalink / raw)
To: Thomas Huth, kvm, Christian Borntraeger
Cc: David Hildenbrand, Cornelia Huck, linux-s390, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 778 bytes --]
On 9/4/19 10:51 AM, Thomas Huth wrote:
> Avoid invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x.
>
> v2:
> - Split the single patch from v1 into two separate patches
> (I've kept the Reviewed-bys from v1, but if you don't agree with the
> patch description of the 2nd patch, please complain)
>
> Thomas Huth (2):
> KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
> KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs
> on s390x
>
> arch/s390/include/uapi/asm/kvm.h | 6 ++++
> arch/s390/kvm/kvm-s390.c | 4 +++
> .../selftests/kvm/s390x/sync_regs_test.c | 30 +++++++++++++++++++
> 3 files changed, 40 insertions(+)
>
Thanks, both are picked
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-09-05 8:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04 8:51 [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Thomas Huth
2019-09-04 8:51 ` [PATCH v2 1/2] KVM: s390: Disallow " Thomas Huth
2019-09-04 9:05 ` David Hildenbrand
2019-09-04 9:10 ` Thomas Huth
2019-09-04 9:11 ` David Hildenbrand
2019-09-04 9:11 ` Christian Borntraeger
2019-09-04 9:15 ` David Hildenbrand
2019-09-04 9:21 ` Thomas Huth
2019-09-04 8:52 ` [PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x Thomas Huth
2019-09-04 9:06 ` David Hildenbrand
2019-09-05 8:00 ` [PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs Janosch Frank
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).