linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix null pointer dereference in kvm_msr_ignored_check
@ 2020-10-25 18:53 Peter Xu
  2020-10-25 18:53 ` [PATCH 1/2] KVM: selftests: Add get featured msrs test case Peter Xu
  2020-10-25 18:53 ` [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS Peter Xu
  0 siblings, 2 replies; 9+ messages in thread
From: Peter Xu @ 2020-10-25 18:53 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Vitaly Kuznetsov, peterx, Sean Christopherson, Paolo Bonzini

Bug report: https://lore.kernel.org/kvm/bug-209845-28872@https.bugzilla.kernel.org%2F/

Unit test attached, which can reproduce the same issue.

Thanks,

Peter Xu (2):
  KVM: selftests: Add get featured msrs test case
  KVM: X86: Fix null pointer reference for KVM_GET_MSRS

 arch/x86/kvm/x86.c                            |  4 +-
 .../testing/selftests/kvm/include/kvm_util.h  |  3 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 14 +++++
 .../testing/selftests/kvm/x86_64/state_test.c | 58 +++++++++++++++++++
 4 files changed, 77 insertions(+), 2 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH 1/2] KVM: selftests: Add get featured msrs test case
  2020-10-25 18:53 [PATCH 0/2] Fix null pointer dereference in kvm_msr_ignored_check Peter Xu
@ 2020-10-25 18:53 ` Peter Xu
  2020-10-26  8:58   ` Vitaly Kuznetsov
  2020-10-25 18:53 ` [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS Peter Xu
  1 sibling, 1 reply; 9+ messages in thread
From: Peter Xu @ 2020-10-25 18:53 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Vitaly Kuznetsov, peterx, Sean Christopherson, Paolo Bonzini

Try to fetch any supported featured msr.  Currently it won't fail, so at least
we can check against valid ones (which should be >0).

This reproduces [1] too by trying to fetch one invalid msr there.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=209845

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  |  3 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 14 +++++
 .../testing/selftests/kvm/x86_64/state_test.c | 58 +++++++++++++++++++
 3 files changed, 75 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 919e161dd289..e34cf263b20a 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -66,6 +66,9 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
 
 struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
 struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
+void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
+				       struct kvm_msr_list *list);
+int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs);
 void kvm_vm_free(struct kvm_vm *vmp);
 void kvm_vm_restart(struct kvm_vm *vmp, int perm);
 void kvm_vm_release(struct kvm_vm *vmp);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 74776ee228f2..3c16fa044335 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -132,6 +132,20 @@ static const struct vm_guest_mode_params vm_guest_mode_params[] = {
 _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
 	       "Missing new mode params?");
 
+void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
+				       struct kvm_msr_list *list)
+{
+	int r = ioctl(vm->kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, list);
+
+	TEST_ASSERT(r == 0, "KVM_GET_MSR_FEATURE_INDEX_LIST failed: %d\n",
+		    -errno);
+}
+
+int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs)
+{
+	return ioctl(vm->kvm_fd, KVM_GET_MSRS, msrs);
+}
+
 /*
  * VM Create
  *
diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
index f6c8b9042f8a..7ce9920e526a 100644
--- a/tools/testing/selftests/kvm/x86_64/state_test.c
+++ b/tools/testing/selftests/kvm/x86_64/state_test.c
@@ -152,6 +152,61 @@ static void __attribute__((__flatten__)) guest_code(void *arg)
 	GUEST_DONE();
 }
 
+#define  KVM_MSR_FEATURE_N  64
+
+static int test_kvm_get_feature_msr_one(struct kvm_vm *vm, __u32 index,
+					struct kvm_msrs *msrs)
+{
+	msrs->nmsrs = 1;
+	msrs->entries[0].index = index;
+	return kvm_vm_get_feature_msrs(vm, msrs);
+}
+
+static void test_kvm_get_msr_features(struct kvm_vm *vm)
+{
+	struct kvm_msr_list *msr_list;
+	struct kvm_msrs *msrs;
+	int i, ret, sum;
+
+	if (!kvm_check_cap(KVM_CAP_GET_MSR_FEATURES)) {
+		pr_info("skipping kvm get msr features test\n");
+		return;
+	}
+
+	msr_list = calloc(1, sizeof(struct kvm_msr_list) +
+			  sizeof(__u32) * KVM_MSR_FEATURE_N);
+	msr_list->nmsrs = KVM_MSR_FEATURE_N;
+
+	TEST_ASSERT(msr_list, "msr_list allocation failed\n");
+
+	kvm_vm_get_msr_feature_index_list(vm, msr_list);
+
+	msrs = calloc(1, sizeof(struct kvm_msrs) +
+		      sizeof(struct kvm_msr_entry));
+
+	TEST_ASSERT(msrs, "msr entries allocation failed\n");
+
+	sum = 0;
+	for (i = 0; i < msr_list->nmsrs; i++) {
+		ret = test_kvm_get_feature_msr_one(vm, msr_list->indices[i],
+						    msrs);
+		TEST_ASSERT(ret >= 0, "KVM_GET_MSR failed: %d\n", ret);
+		sum += ret;
+	}
+	TEST_ASSERT(sum > 0, "KVM_GET_MSR has no feature msr\n");
+
+	/*
+	 * Test invalid msr.  Note the retcode can be either 0 or 1 depending
+	 * on kvm.ignore_msrs
+	 */
+	ret = test_kvm_get_feature_msr_one(vm, (__u32)-1, msrs);
+	TEST_ASSERT(ret >= 0 && ret <= 1,
+		    "KVM_GET_MSR on invalid msr error: %d\n", ret);
+
+	free(msrs);
+	free(msr_list);
+}
+
 int main(int argc, char *argv[])
 {
 	vm_vaddr_t nested_gva = 0;
@@ -168,6 +223,9 @@ int main(int argc, char *argv[])
 	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
 	run = vcpu_state(vm, VCPU_ID);
 
+	/* Test KVM_GET_MSR for VM */
+	test_kvm_get_msr_features(vm);
+
 	vcpu_regs_get(vm, VCPU_ID, &regs1);
 
 	if (kvm_check_cap(KVM_CAP_NESTED_STATE)) {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS
  2020-10-25 18:53 [PATCH 0/2] Fix null pointer dereference in kvm_msr_ignored_check Peter Xu
  2020-10-25 18:53 ` [PATCH 1/2] KVM: selftests: Add get featured msrs test case Peter Xu
@ 2020-10-25 18:53 ` Peter Xu
  2020-10-26  9:07   ` Vitaly Kuznetsov
  2020-10-31 14:06   ` Paolo Bonzini
  1 sibling, 2 replies; 9+ messages in thread
From: Peter Xu @ 2020-10-25 18:53 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Vitaly Kuznetsov, peterx, Sean Christopherson, Paolo Bonzini,
	Steffen Dirkwinkel

kvm_msr_ignored_check() could trigger a null pointer reference if ignore_msrs=Y
and report_ignore_msrs=Y when try to fetch an invalid feature msr using the
global KVM_GET_MSRS.  Degrade the error report to not rely on vcpu since that
information (index, rip) is not as important as msr index/data after all.

Fixes: 12bc2132b15e0a96
Reported-by: Steffen Dirkwinkel <kernel-bugs@steffen.cc>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ce856e0ece84..5993fbd6d2c5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -259,8 +259,8 @@ static int kvm_msr_ignored_check(struct kvm_vcpu *vcpu, u32 msr,
 
 	if (ignore_msrs) {
 		if (report_ignored_msrs)
-			vcpu_unimpl(vcpu, "ignored %s: 0x%x data 0x%llx\n",
-				    op, msr, data);
+			kvm_pr_unimpl("ignored %s: 0x%x data 0x%llx\n",
+				      op, msr, data);
 		/* Mask the error */
 		return 0;
 	} else {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Add get featured msrs test case
  2020-10-25 18:53 ` [PATCH 1/2] KVM: selftests: Add get featured msrs test case Peter Xu
@ 2020-10-26  8:58   ` Vitaly Kuznetsov
  2020-10-26  9:08     ` Andrew Jones
  2020-10-31 15:34     ` Peter Xu
  0 siblings, 2 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2020-10-26  8:58 UTC (permalink / raw)
  To: Peter Xu; +Cc: peterx, Sean Christopherson, Paolo Bonzini, linux-kernel, kvm

Peter Xu <peterx@redhat.com> writes:

> Try to fetch any supported featured msr.  Currently it won't fail, so at least
> we can check against valid ones (which should be >0).
>
> This reproduces [1] too by trying to fetch one invalid msr there.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=209845
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  .../testing/selftests/kvm/include/kvm_util.h  |  3 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 14 +++++
>  .../testing/selftests/kvm/x86_64/state_test.c | 58 +++++++++++++++++++
>  3 files changed, 75 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 919e161dd289..e34cf263b20a 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -66,6 +66,9 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
>  
>  struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
>  struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
> +void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
> +				       struct kvm_msr_list *list);
> +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs);
>  void kvm_vm_free(struct kvm_vm *vmp);
>  void kvm_vm_restart(struct kvm_vm *vmp, int perm);
>  void kvm_vm_release(struct kvm_vm *vmp);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 74776ee228f2..3c16fa044335 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -132,6 +132,20 @@ static const struct vm_guest_mode_params vm_guest_mode_params[] = {
>  _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
>  	       "Missing new mode params?");
>  
> +void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
> +				       struct kvm_msr_list *list)
> +{
> +	int r = ioctl(vm->kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, list);
> +
> +	TEST_ASSERT(r == 0, "KVM_GET_MSR_FEATURE_INDEX_LIST failed: %d\n",
> +		    -errno);
> +}
> +
> +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs)
> +{
> +	return ioctl(vm->kvm_fd, KVM_GET_MSRS, msrs);
> +}

I *think* that the non-written rule for kvm selftests is that functions
without '_' prefix check ioctl return value with TEST_ASSERT() and
functions with it don't (e.g. _vcpu_run()/vcpu_run()) but maybe it's
just me.

> +
>  /*
>   * VM Create
>   *
> diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
> index f6c8b9042f8a..7ce9920e526a 100644
> --- a/tools/testing/selftests/kvm/x86_64/state_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/state_test.c

I would not overload state_test with this new check and create a new
one. The benefit is that when one of these tests fail we still get the
result of the other one so it's not 'everything works' vs 'everything is
broken' type of log.

> @@ -152,6 +152,61 @@ static void __attribute__((__flatten__)) guest_code(void *arg)
>  	GUEST_DONE();
>  }
>  
> +#define  KVM_MSR_FEATURE_N  64
> +
> +static int test_kvm_get_feature_msr_one(struct kvm_vm *vm, __u32 index,
> +					struct kvm_msrs *msrs)
> +{
> +	msrs->nmsrs = 1;
> +	msrs->entries[0].index = index;
> +	return kvm_vm_get_feature_msrs(vm, msrs);
> +}
> +
> +static void test_kvm_get_msr_features(struct kvm_vm *vm)
> +{
> +	struct kvm_msr_list *msr_list;
> +	struct kvm_msrs *msrs;
> +	int i, ret, sum;
> +
> +	if (!kvm_check_cap(KVM_CAP_GET_MSR_FEATURES)) {
> +		pr_info("skipping kvm get msr features test\n");
> +		return;
> +	}
> +
> +	msr_list = calloc(1, sizeof(struct kvm_msr_list) +
> +			  sizeof(__u32) * KVM_MSR_FEATURE_N);
> +	msr_list->nmsrs = KVM_MSR_FEATURE_N;
> +
> +	TEST_ASSERT(msr_list, "msr_list allocation failed\n");
> +
> +	kvm_vm_get_msr_feature_index_list(vm, msr_list);
> +
> +	msrs = calloc(1, sizeof(struct kvm_msrs) +
> +		      sizeof(struct kvm_msr_entry));
> +
> +	TEST_ASSERT(msrs, "msr entries allocation failed\n");
> +
> +	sum = 0;
> +	for (i = 0; i < msr_list->nmsrs; i++) {
> +		ret = test_kvm_get_feature_msr_one(vm, msr_list->indices[i],
> +						    msrs);
> +		TEST_ASSERT(ret >= 0, "KVM_GET_MSR failed: %d\n", ret);
> +		sum += ret;
> +	}
> +	TEST_ASSERT(sum > 0, "KVM_GET_MSR has no feature msr\n");
> +
> +	/*
> +	 * Test invalid msr.  Note the retcode can be either 0 or 1 depending
> +	 * on kvm.ignore_msrs
> +	 */
> +	ret = test_kvm_get_feature_msr_one(vm, (__u32)-1, msrs);
> +	TEST_ASSERT(ret >= 0 && ret <= 1,
> +		    "KVM_GET_MSR on invalid msr error: %d\n", ret);
> +
> +	free(msrs);
> +	free(msr_list);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	vm_vaddr_t nested_gva = 0;
> @@ -168,6 +223,9 @@ int main(int argc, char *argv[])
>  	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
>  	run = vcpu_state(vm, VCPU_ID);
>  
> +	/* Test KVM_GET_MSR for VM */
> +	test_kvm_get_msr_features(vm);
> +
>  	vcpu_regs_get(vm, VCPU_ID, &regs1);
>  
>  	if (kvm_check_cap(KVM_CAP_NESTED_STATE)) {

-- 
Vitaly


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS
  2020-10-25 18:53 ` [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS Peter Xu
@ 2020-10-26  9:07   ` Vitaly Kuznetsov
  2020-10-31 14:06   ` Paolo Bonzini
  1 sibling, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2020-10-26  9:07 UTC (permalink / raw)
  To: Peter Xu, linux-kernel, kvm
  Cc: peterx, Sean Christopherson, Paolo Bonzini, Steffen Dirkwinkel

Peter Xu <peterx@redhat.com> writes:

> kvm_msr_ignored_check() could trigger a null pointer reference

'dereference' but I'd also clarify that 'vcpu' is NULL.

>  if ignore_msrs=Y
> and report_ignore_msrs=Y when try to fetch an invalid feature msr using the
> global KVM_GET_MSRS.  Degrade the error report to not rely on vcpu since that
> information (index, rip) is not as important as msr index/data after all.
>
> Fixes: 12bc2132b15e0a96

Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")

please (checkpatch.pl should've complained I guess)

> Reported-by: Steffen Dirkwinkel <kernel-bugs@steffen.cc>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ce856e0ece84..5993fbd6d2c5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -259,8 +259,8 @@ static int kvm_msr_ignored_check(struct kvm_vcpu *vcpu, u32 msr,
>  
>  	if (ignore_msrs) {
>  		if (report_ignored_msrs)
> -			vcpu_unimpl(vcpu, "ignored %s: 0x%x data 0x%llx\n",
> -				    op, msr, data);
> +			kvm_pr_unimpl("ignored %s: 0x%x data 0x%llx\n",
> +				      op, msr, data);

I would've preserved vcpu version for non-gloal cases, e.g.

if (report_ignored_msrs) {
	if (vcpu)
        	vcpu_unimpl(vcpu, "ignored %s: 0x%x data 0x%llx\n",
	                op, msr, data);
        else
	        kvm_pr_unimpl("ignored %s: 0x%x data 0x%llx\n",
                               op, msr, data);
}

>  		/* Mask the error */
>  		return 0;
>  	} else {

-- 
Vitaly


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Add get featured msrs test case
  2020-10-26  8:58   ` Vitaly Kuznetsov
@ 2020-10-26  9:08     ` Andrew Jones
  2020-10-31 15:34     ` Peter Xu
  1 sibling, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2020-10-26  9:08 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Peter Xu, Sean Christopherson, Paolo Bonzini, linux-kernel, kvm

On Mon, Oct 26, 2020 at 09:58:54AM +0100, Vitaly Kuznetsov wrote:
> Peter Xu <peterx@redhat.com> writes:
> > +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs)
> > +{
> > +	return ioctl(vm->kvm_fd, KVM_GET_MSRS, msrs);
> > +}
> 
> I *think* that the non-written rule for kvm selftests is that functions
> without '_' prefix check ioctl return value with TEST_ASSERT() and
> functions with it don't (e.g. _vcpu_run()/vcpu_run()) but maybe it's
> just me.
>

Yes, that's the pattern I've been trying to implement. If we want to be
strict about it, then we should do a quick scan of the code to ensure
its currently consistent. I have it feeling it isn't.

Thanks,
drew


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS
  2020-10-25 18:53 ` [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS Peter Xu
  2020-10-26  9:07   ` Vitaly Kuznetsov
@ 2020-10-31 14:06   ` Paolo Bonzini
  2020-10-31 15:27     ` Peter Xu
  1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-10-31 14:06 UTC (permalink / raw)
  To: Peter Xu, linux-kernel, kvm, Takashi Iwai
  Cc: Vitaly Kuznetsov, Sean Christopherson, Steffen Dirkwinkel

On 25/10/20 19:53, Peter Xu wrote:
> kvm_msr_ignored_check() could trigger a null pointer reference if ignore_msrs=Y
> and report_ignore_msrs=Y when try to fetch an invalid feature msr using the
> global KVM_GET_MSRS.  Degrade the error report to not rely on vcpu since that
> information (index, rip) is not as important as msr index/data after all.
> 
> Fixes: 12bc2132b15e0a96
> Reported-by: Steffen Dirkwinkel <kernel-bugs@steffen.cc>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ce856e0ece84..5993fbd6d2c5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -259,8 +259,8 @@ static int kvm_msr_ignored_check(struct kvm_vcpu *vcpu, u32 msr,
>  
>  	if (ignore_msrs) {
>  		if (report_ignored_msrs)
> -			vcpu_unimpl(vcpu, "ignored %s: 0x%x data 0x%llx\n",
> -				    op, msr, data);
> +			kvm_pr_unimpl("ignored %s: 0x%x data 0x%llx\n",
> +				      op, msr, data);
>  		/* Mask the error */
>  		return 0;
>  	} else {
> 

I committed Takashi Iwai's very similar patch.  Please resend 1/2 with
reviewer comments addressed, thanks!

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS
  2020-10-31 14:06   ` Paolo Bonzini
@ 2020-10-31 15:27     ` Peter Xu
  0 siblings, 0 replies; 9+ messages in thread
From: Peter Xu @ 2020-10-31 15:27 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: linux-kernel, kvm, Takashi Iwai, Vitaly Kuznetsov,
	Sean Christopherson, Steffen Dirkwinkel

On Sat, Oct 31, 2020 at 03:06:59PM +0100, Paolo Bonzini wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index ce856e0ece84..5993fbd6d2c5 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -259,8 +259,8 @@ static int kvm_msr_ignored_check(struct kvm_vcpu *vcpu, u32 msr,
> >  
> >  	if (ignore_msrs) {
> >  		if (report_ignored_msrs)
> > -			vcpu_unimpl(vcpu, "ignored %s: 0x%x data 0x%llx\n",
> > -				    op, msr, data);
> > +			kvm_pr_unimpl("ignored %s: 0x%x data 0x%llx\n",
> > +				      op, msr, data);
> >  		/* Mask the error */
> >  		return 0;
> >  	} else {
> > 
> 
> I committed Takashi Iwai's very similar patch.  Please resend 1/2 with
> reviewer comments addressed, thanks!

Sorry for a late reply (to reviewers).

Oh, how did I miss the other vcpu reference... that one is definitely better. :)

Will respin shortly on the test.

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Add get featured msrs test case
  2020-10-26  8:58   ` Vitaly Kuznetsov
  2020-10-26  9:08     ` Andrew Jones
@ 2020-10-31 15:34     ` Peter Xu
  1 sibling, 0 replies; 9+ messages in thread
From: Peter Xu @ 2020-10-31 15:34 UTC (permalink / raw)
  To: Vitaly Kuznetsov; +Cc: Sean Christopherson, Paolo Bonzini, linux-kernel, kvm

On Mon, Oct 26, 2020 at 09:58:54AM +0100, Vitaly Kuznetsov wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Try to fetch any supported featured msr.  Currently it won't fail, so at least
> > we can check against valid ones (which should be >0).
> >
> > This reproduces [1] too by trying to fetch one invalid msr there.
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=209845
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  .../testing/selftests/kvm/include/kvm_util.h  |  3 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 14 +++++
> >  .../testing/selftests/kvm/x86_64/state_test.c | 58 +++++++++++++++++++
> >  3 files changed, 75 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 919e161dd289..e34cf263b20a 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -66,6 +66,9 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
> >  
> >  struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
> >  struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
> > +void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
> > +				       struct kvm_msr_list *list);
> > +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs);
> >  void kvm_vm_free(struct kvm_vm *vmp);
> >  void kvm_vm_restart(struct kvm_vm *vmp, int perm);
> >  void kvm_vm_release(struct kvm_vm *vmp);
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 74776ee228f2..3c16fa044335 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -132,6 +132,20 @@ static const struct vm_guest_mode_params vm_guest_mode_params[] = {
> >  _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
> >  	       "Missing new mode params?");
> >  
> > +void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
> > +				       struct kvm_msr_list *list)
> > +{
> > +	int r = ioctl(vm->kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, list);
> > +
> > +	TEST_ASSERT(r == 0, "KVM_GET_MSR_FEATURE_INDEX_LIST failed: %d\n",
> > +		    -errno);
> > +}
> > +
> > +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs)
> > +{
> > +	return ioctl(vm->kvm_fd, KVM_GET_MSRS, msrs);
> > +}
> 
> I *think* that the non-written rule for kvm selftests is that functions
> without '_' prefix check ioctl return value with TEST_ASSERT() and
> functions with it don't (e.g. _vcpu_run()/vcpu_run()) but maybe it's
> just me.

Sure, will fix it up.

> 
> > +
> >  /*
> >   * VM Create
> >   *
> > diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
> > index f6c8b9042f8a..7ce9920e526a 100644
> > --- a/tools/testing/selftests/kvm/x86_64/state_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/state_test.c
> 
> I would not overload state_test with this new check and create a new
> one. The benefit is that when one of these tests fail we still get the
> result of the other one so it's not 'everything works' vs 'everything is
> broken' type of log.

IMHO it's not extremely important on knowingg which binary failed - afaiu,
kernel selftests are really for an "all pass", so if anything fails, we dig.

Another thing, state_test.c has a comment (at the top):

/*
 * KVM_GET/SET_* tests
 *
 * Copyright (C) 2018, Red Hat, Inc.
 ...

Shouldn't KVM_GET_MSRS suites here? :)

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-31 15:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-25 18:53 [PATCH 0/2] Fix null pointer dereference in kvm_msr_ignored_check Peter Xu
2020-10-25 18:53 ` [PATCH 1/2] KVM: selftests: Add get featured msrs test case Peter Xu
2020-10-26  8:58   ` Vitaly Kuznetsov
2020-10-26  9:08     ` Andrew Jones
2020-10-31 15:34     ` Peter Xu
2020-10-25 18:53 ` [PATCH 2/2] KVM: X86: Fix null pointer reference for KVM_GET_MSRS Peter Xu
2020-10-26  9:07   ` Vitaly Kuznetsov
2020-10-31 14:06   ` Paolo Bonzini
2020-10-31 15:27     ` Peter Xu

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).