[RFC PATCH v2 0/3] MSR filtering / exiting flag cleanup

[RFC PATCH v2 0/3] MSR filtering / exiting flag cleanup

[Aaron Lewis]

Posting as an RFC to get feedback whether it's too late to protect the
unused flag bits.  My hope is this feature is still new enough, and not
widely used enough, and this change is reasonable enough to be able to be
corrected.  These bits should have been protected from the start, but
unfortunately they were not.

Other approaches to fixing this could be to fix it with a quirk, or the
tried and true KVM method of adding a "2" (e.g. KVM_CAP_X86_USER_SPACE_MSR2).
Both approaches, however, complicate the code more than it would otherwise
be if the original feature could be patched.

For long term simplicity my hope is to be able to just patch
the original change.

Note: the second patch in this series does not contain any functional changes,
so it is not an RFC.  The others do, so they are.

v1 -> v2
 - Added valid masks KVM_MSR_FILTER_VALID_MASK and
   KVM_MSR_EXIT_REASON_VALID_MASK.
 - Added patch 2/3 to add valid mask KVM_MSR_FILTER_RANGE_VALID_MASK, and
   use it.
 - Added testing to demonstrate flag protection when calling the ioctl for
   KVM_X86_SET_MSR_FILTER or KVM_CAP_X86_USER_SPACE_MSR.

Aaron Lewis (3):
  KVM: x86: Protect the unused bits in the MSR filtering / exiting flags
  KVM: x86: Add a VALID_MASK for the flags in kvm_msr_filter_range
  selftests: kvm/x86: Test the flags in MSR filtering / exiting

 arch/x86/include/uapi/asm/kvm.h               |  3 +
 arch/x86/kvm/x86.c                            |  8 +-
 include/uapi/linux/kvm.h                      |  3 +
 .../kvm/x86_64/userspace_msr_exit_test.c      | 95 +++++++++++++++++++
 4 files changed, 108 insertions(+), 1 deletion(-)

-- 
2.37.1.359.gd136c6c3e2-goog

[RFC PATCH v2 1/3] KVM: x86: Protect the unused bits in the MSR filtering / exiting flags

[Aaron Lewis]

The flags used in KVM_CAP_X86_USER_SPACE_MSR and KVM_X86_SET_MSR_FILTER
have no protection for their unused bits.  Without protection, future
development for these features will be difficult.  Add the protection
needed to make it possible to extend these features in the future.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/include/uapi/asm/kvm.h | 1 +
 arch/x86/kvm/x86.c              | 6 ++++++
 include/uapi/linux/kvm.h        | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index ee3896416c68..63691a4c62d0 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -224,6 +224,7 @@ struct kvm_msr_filter_range {
 struct kvm_msr_filter {
 #define KVM_MSR_FILTER_DEFAULT_ALLOW (0 << 0)
 #define KVM_MSR_FILTER_DEFAULT_DENY  (1 << 0)
+#define KVM_MSR_FILTER_VALID_MASK (KVM_MSR_FILTER_DEFAULT_DENY)
 	__u32 flags;
 	struct kvm_msr_filter_range ranges[KVM_MSR_FILTER_MAX_RANGES];
 };
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 031678eff28e..adaec8d07a25 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6171,6 +6171,9 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		r = 0;
 		break;
 	case KVM_CAP_X86_USER_SPACE_MSR:
+		r = -EINVAL;
+		if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK)
+			break;
 		kvm->arch.user_space_msr_mask = cap->args[0];
 		r = 0;
 		break;
@@ -6384,6 +6387,9 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
 	if (copy_from_user(&filter, user_msr_filter, sizeof(filter)))
 		return -EFAULT;
 
+	if (filter.flags & ~KVM_MSR_FILTER_VALID_MASK)
+		return -EINVAL;
+
 	for (i = 0; i < ARRAY_SIZE(filter.ranges); i++)
 		empty &= !filter.ranges[i].nmsrs;
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index a36e78710382..236b8e09eef1 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -484,6 +484,9 @@ struct kvm_run {
 #define KVM_MSR_EXIT_REASON_INVAL	(1 << 0)
 #define KVM_MSR_EXIT_REASON_UNKNOWN	(1 << 1)
 #define KVM_MSR_EXIT_REASON_FILTER	(1 << 2)
+#define KVM_MSR_EXIT_REASON_VALID_MASK	(KVM_MSR_EXIT_REASON_INVAL   |	\
+					 KVM_MSR_EXIT_REASON_UNKNOWN |	\
+					 KVM_MSR_EXIT_REASON_FILTER)
 			__u32 reason; /* kernel -> user */
 			__u32 index; /* kernel -> user */
 			__u64 data; /* kernel <-> user */
-- 
2.37.1.359.gd136c6c3e2-goog

[PATCH v2 2/3] KVM: x86: Add a VALID_MASK for the flags in kvm_msr_filter_range

[Aaron Lewis]

Add the mask KVM_MSR_FILTER_RANGE_VALID_MASK for the flags in the
struct kvm_msr_filter_range.  This simplifies checks that validate
these flags, and makes it easier to introduce new flags in the future.

No functional change intended.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/include/uapi/asm/kvm.h | 2 ++
 arch/x86/kvm/x86.c              | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 63691a4c62d0..a9fecd3eab61 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -214,6 +214,8 @@ struct kvm_msr_list {
 struct kvm_msr_filter_range {
 #define KVM_MSR_FILTER_READ  (1 << 0)
 #define KVM_MSR_FILTER_WRITE (1 << 1)
+#define KVM_MSR_FILTER_RANGE_VALID_MASK (KVM_MSR_FILTER_READ | \
+					 KVM_MSR_FILTER_WRITE)
 	__u32 flags;
 	__u32 nmsrs; /* number of msrs in bitmap */
 	__u32 base;  /* MSR index the bitmap starts at */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index adaec8d07a25..6c1a531e3b88 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6349,7 +6349,7 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter,
 	if (!user_range->nmsrs)
 		return 0;
 
-	if (user_range->flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE))
+	if (user_range->flags & ~KVM_MSR_FILTER_RANGE_VALID_MASK)
 		return -EINVAL;
 
 	if (!user_range->flags)
-- 
2.37.1.359.gd136c6c3e2-goog

[RFC PATCH v2 3/3] selftests: kvm/x86: Test the flags in MSR filtering / exiting

[Aaron Lewis]

When using the flags in KVM_X86_SET_MSR_FILTER and
KVM_CAP_X86_USER_SPACE_MSR it is expected that an attempt to write to
any of the unused bits will fail.  Add testing to walk over every bit
in each of the flag fields in MSR filtering / exiting to verify that
happens.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../kvm/x86_64/userspace_msr_exit_test.c      | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
index f84dc37426f5..3b4ad16cc982 100644
--- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
+++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
@@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void)
 	kvm_vm_free(vm);
 }
 
+static void test_results(int rc, const char *scmd, bool expected_success)
+{
+	int expected_rc;
+
+	expected_rc = expected_success ? 0 : -1;
+	TEST_ASSERT(rc == expected_rc,
+		    "Unexpected result from '%s', rc: %d, expected rc: %d.",
+		    scmd, rc, expected_rc);
+	TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL),
+		    "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n"
+		    "  got rc: %d, errno: %d",
+		    EINVAL, rc, errno);
+}
+
+#define test_ioctl(vm, cmd, arg, expected_success)	\
+({							\
+	int rc = __vm_ioctl(vm, cmd, arg);		\
+							\
+	test_results(rc, #cmd, expected_success);	\
+})
+#define FLAG (1ul << i)
+
+static void run_user_space_msr_flag_test(struct kvm_vm *vm)
+{
+	struct kvm_enable_cap cap = { .cap = KVM_CAP_X86_USER_SPACE_MSR };
+	int nflags = sizeof(cap.args[0]) * BITS_PER_BYTE;
+	int rc;
+	int i;
+
+	rc = kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR);
+	TEST_ASSERT(rc, "KVM_CAP_X86_USER_SPACE_MSR is available");
+
+	for (i = 0; i < nflags; i++) {
+		cap.args[0] = FLAG;
+		test_ioctl(vm, KVM_ENABLE_CAP, &cap,
+			   FLAG & KVM_MSR_EXIT_REASON_VALID_MASK);
+	}
+}
+
+static void run_msr_filter_flag_test(struct kvm_vm *vm)
+{
+	u64 deny_bits = 0;
+	struct kvm_msr_filter filter = {
+		.flags = KVM_MSR_FILTER_DEFAULT_ALLOW,
+		.ranges = {
+			{
+				.flags = KVM_MSR_FILTER_READ,
+				.nmsrs = 1,
+				.base = 0,
+				.bitmap = (uint8_t *)&deny_bits,
+			},
+		},
+	};
+	int nflags;
+	int rc;
+	int i;
+
+	rc = kvm_check_cap(KVM_CAP_X86_MSR_FILTER);
+	TEST_ASSERT(rc, "KVM_CAP_X86_MSR_FILTER is available");
+
+	nflags = sizeof(filter.flags) * BITS_PER_BYTE;
+	for (i = 0; i < nflags; i++) {
+		filter.flags = FLAG;
+		test_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter,
+			   FLAG & KVM_MSR_FILTER_VALID_MASK);
+	}
+
+	filter.flags = KVM_MSR_FILTER_DEFAULT_ALLOW;
+	nflags = sizeof(filter.ranges[0].flags) * BITS_PER_BYTE;
+	for (i = 0; i < nflags; i++) {
+		filter.ranges[0].flags = FLAG;
+		test_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter,
+			   FLAG & KVM_MSR_FILTER_RANGE_VALID_MASK);
+	}
+}
+
+/* Test that attempts to write to the unused bits in a flag fails. */
+static void test_flags(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, NULL);
+
+	/* Test flags for KVM_CAP_X86_USER_SPACE_MSR. */
+	run_user_space_msr_flag_test(vm);
+
+	/* Test flags and range flags for KVM_X86_SET_MSR_FILTER. */
+	run_msr_filter_flag_test(vm);
+
+	kvm_vm_free(vm);
+}
+
 int main(int argc, char *argv[])
 {
 	/* Tell stdout not to buffer its content */
@@ -745,5 +838,7 @@ int main(int argc, char *argv[])
 
 	test_msr_permission_bitmap();
 
+	test_flags();
+
 	return 0;
 }
-- 
2.37.1.359.gd136c6c3e2-goog

Re: [RFC PATCH v2 3/3] selftests: kvm/x86: Test the flags in MSR filtering / exiting

[Sean Christopherson]

On Tue, Jul 19, 2022, Aaron Lewis wrote:
> When using the flags in KVM_X86_SET_MSR_FILTER and
> KVM_CAP_X86_USER_SPACE_MSR it is expected that an attempt to write to
> any of the unused bits will fail.  Add testing to walk over every bit
> in each of the flag fields in MSR filtering / exiting to verify that
> happens.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  .../kvm/x86_64/userspace_msr_exit_test.c      | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> index f84dc37426f5..3b4ad16cc982 100644
> --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void)
>  	kvm_vm_free(vm);
>  }
>  
> +static void test_results(int rc, const char *scmd, bool expected_success)

Rather than pass in "success expected", pass in the actual value and the valid
mask.  Then you can spit out the problematic value in the assert and be kind to
future debuggers.

And similarly, make the __vm_ioctl() call here instead of in the "caller" and name
this __test_ioctl() (rename as necessary, see below) to show it's relationship with
the macro.

> +{
> +	int expected_rc;
> +
> +	expected_rc = expected_success ? 0 : -1;
> +	TEST_ASSERT(rc == expected_rc,
> +		    "Unexpected result from '%s', rc: %d, expected rc: %d.",
> +		    scmd, rc, expected_rc);
> +	TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL),
> +		    "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n"
> +		    "  got rc: %d, errno: %d",
> +		    EINVAL, rc, errno);
> +}
> +
> +#define test_ioctl(vm, cmd, arg, expected_success)	\

As above, just do e.g.

#define test_ioctl(vm, cmd, arg, val, valid_mask)	\
	__test_ioctl(vm, cmd, arg, #cmd, val, valid_mask)

Though it might be worth using a more verbose name?  E.g. test_msr_filtering_ioctl()?
Hmm, but I guess KVM_CAP_X86_USER_SPACE_MSR isn't technically filtering.
test_user_exit_msr_ioctl()?  Not a big deal if that's too wordy.

> +({							\
> +	int rc = __vm_ioctl(vm, cmd, arg);		\
> +							\
> +	test_results(rc, #cmd, expected_success);	\
> +})
> +#define FLAG (1ul << i)

No.  :-)

First, silently consuming local variables is Evil with a capital E.

Second, just use BIT() or BIT_ULL().

> +/* Test that attempts to write to the unused bits in a flag fails. */
> +static void test_flags(void)

For this one, definitely use a more verbose name, even if it seems stupidly
redundant.  test_user_msr_exit_flags()?

> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, NULL);
> +
> +	/* Test flags for KVM_CAP_X86_USER_SPACE_MSR. */
> +	run_user_space_msr_flag_test(vm);
> +
> +	/* Test flags and range flags for KVM_X86_SET_MSR_FILTER. */
> +	run_msr_filter_flag_test(vm);
> +
> +	kvm_vm_free(vm);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	/* Tell stdout not to buffer its content */
> @@ -745,5 +838,7 @@ int main(int argc, char *argv[])
>  
>  	test_msr_permission_bitmap();
>  
> +	test_flags();
> +
>  	return 0;
>  }
> -- 
> 2.37.1.359.gd136c6c3e2-goog
> 

Re: [RFC PATCH v2 1/3] KVM: x86: Protect the unused bits in the MSR filtering / exiting flags

[Sean Christopherson]

On Tue, Jul 19, 2022, Aaron Lewis wrote:
> The flags used in KVM_CAP_X86_USER_SPACE_MSR and KVM_X86_SET_MSR_FILTER
> have no protection for their unused bits.  Without protection, future
> development for these features will be difficult.  Add the protection
> needed to make it possible to extend these features in the future.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/include/uapi/asm/kvm.h | 1 +
>  arch/x86/kvm/x86.c              | 6 ++++++
>  include/uapi/linux/kvm.h        | 3 +++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index ee3896416c68..63691a4c62d0 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -224,6 +224,7 @@ struct kvm_msr_filter_range {
>  struct kvm_msr_filter {
>  #define KVM_MSR_FILTER_DEFAULT_ALLOW (0 << 0)

Well this is silly.  Can we wrap this with

#ifdef __KERNEL__
#define KVM_MSR_FILTER_DEFAULT_ALLOW (0 << 0)
#endif

so that we don't try to use it in the kernel?  E.g. I can see someone doing

	if (filter.flags & KVM_MSR_FILTER_DEFAULT_ALLOW)
		<allow the MSR>

and getting really confused when that doesn't work.

Or if we're feeling lucky, just remove it entirely as userspace doing

	filter.flags &= KVM_MSR_FILTER_DEFAULT_ALLOW;

is going to make someone sad someday.

>  #define KVM_MSR_FILTER_DEFAULT_DENY  (1 << 0)
> +#define KVM_MSR_FILTER_VALID_MASK (KVM_MSR_FILTER_DEFAULT_DENY)
>  	__u32 flags;
>  	struct kvm_msr_filter_range ranges[KVM_MSR_FILTER_MAX_RANGES];
>  };

Re: [RFC PATCH v2 3/3] selftests: kvm/x86: Test the flags in MSR filtering / exiting

[Aaron Lewis]

> > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void)
> >       kvm_vm_free(vm);
> >  }
> >
> > +static void test_results(int rc, const char *scmd, bool expected_success)
>
> Rather than pass in "success expected", pass in the actual value and the valid
> mask.  Then you can spit out the problematic value in the assert and be kind to
> future debuggers.
>
> And similarly, make the __vm_ioctl() call here instead of in the "caller" and name
> this __test_ioctl() (rename as necessary, see below) to show it's relationship with
> the macro.

The other comments look good.  I'll update.

This one is a bit tricky though.  I did originally have __vm_ioctl()
in test_results() (or whatever name it will end up with), but the
static assert in kvm_do_ioctl() gave me problems.  Unless I make
test_results() a macro, I have to force cmd to a uint64_t or something
other than a literal, then I get this:

include/kvm_util_base.h:190:39: error: expression in static assertion
is not constant
190 |         static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) ==
_IOC_SIZE(cmd), "");   \
       |                                       ^
include/kvm_util_base.h:213:9: note: in expansion of macro ‘kvm_do_ioctl’
213 |         kvm_do_ioctl((vm)->fd, cmd, arg);                       \

That's not the only problem.  In order to pass 'arg' in I would have
to pass it as a void *, making sizeof(*arg) wrong.

Being that the ioctl call was the first thing I did in that function I
opted to make it a part of test_ioctl() rather than making
test_results() a macro.

If only C had templates :)

>
> > +{
> > +     int expected_rc;
> > +
> > +     expected_rc = expected_success ? 0 : -1;
> > +     TEST_ASSERT(rc == expected_rc,
> > +                 "Unexpected result from '%s', rc: %d, expected rc: %d.",
> > +                 scmd, rc, expected_rc);
> > +     TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL),
> > +                 "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n"
> > +                 "  got rc: %d, errno: %d",
> > +                 EINVAL, rc, errno);
> > +}
> > +
> > +#define test_ioctl(vm, cmd, arg, expected_success)   \
>

Re: [RFC PATCH v2 3/3] selftests: kvm/x86: Test the flags in MSR filtering / exiting

[Sean Christopherson]

On Thu, Jul 21, 2022, Aaron Lewis wrote:
> > > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void)
> > >       kvm_vm_free(vm);
> > >  }
> > >
> > > +static void test_results(int rc, const char *scmd, bool expected_success)
> >
> > Rather than pass in "success expected", pass in the actual value and the valid
> > mask.  Then you can spit out the problematic value in the assert and be kind to
> > future debuggers.
> >
> > And similarly, make the __vm_ioctl() call here instead of in the "caller" and name
> > this __test_ioctl() (rename as necessary, see below) to show it's relationship with
> > the macro.
> 
> The other comments look good.  I'll update.
> 
> This one is a bit tricky though.  I did originally have __vm_ioctl()
> in test_results() (or whatever name it will end up with), but the
> static assert in kvm_do_ioctl() gave me problems.  Unless I make
> test_results() a macro, I have to force cmd to a uint64_t or something
> other than a literal, then I get this:
> 
> include/kvm_util_base.h:190:39: error: expression in static assertion
> is not constant
> 190 |         static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) ==
> _IOC_SIZE(cmd), "");   \
>        |                                       ^
> include/kvm_util_base.h:213:9: note: in expansion of macro ‘kvm_do_ioctl’
> 213 |         kvm_do_ioctl((vm)->fd, cmd, arg);                       \
> 
> That's not the only problem.  In order to pass 'arg' in I would have
> to pass it as a void *, making sizeof(*arg) wrong.
> 
> Being that the ioctl call was the first thing I did in that function I
> opted to make it a part of test_ioctl() rather than making
> test_results() a macro.
> 
> If only C had templates :)

Eh, what C++ can do with templates, C can usually do with macros :-)

Sans backslashes, I think this can simply be:

#define test_user_exit_msr_ioctl(vm, cmd, arg, val, valid_mask)
({
	int r = __vm_ioctl(vm, cmd, arg);

	if (val & valid_mask)
		TEST_ASSERT(!r, KVM_IOCTL_ERROR(cmd, r));
	else
		TEST_ASSERT(r == -1 && errno == EINVAL,
			    "Wanted EINVAL with val = 0x%llx, got  rc: %i errno: %i (%s)",
			    val, r, errno,  strerror(errno))
})


It doesn't print "val" when success is expected, but I'm ok with that.  Though I
suspect that adding a common macro to print additional info on an unexpected
ioctl() result would be useful for other tests.

Re: [RFC PATCH v2 1/3] KVM: x86: Protect the unused bits in the MSR filtering / exiting flags

[Aaron Lewis]

On Wed, Jul 20, 2022 at 11:31 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Jul 19, 2022, Aaron Lewis wrote:
> > The flags used in KVM_CAP_X86_USER_SPACE_MSR and KVM_X86_SET_MSR_FILTER
> > have no protection for their unused bits.  Without protection, future
> > development for these features will be difficult.  Add the protection
> > needed to make it possible to extend these features in the future.
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> >  arch/x86/include/uapi/asm/kvm.h | 1 +
> >  arch/x86/kvm/x86.c              | 6 ++++++
> >  include/uapi/linux/kvm.h        | 3 +++
> >  3 files changed, 10 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index ee3896416c68..63691a4c62d0 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -224,6 +224,7 @@ struct kvm_msr_filter_range {
> >  struct kvm_msr_filter {
> >  #define KVM_MSR_FILTER_DEFAULT_ALLOW (0 << 0)
>
> Well this is silly.  Can we wrap this with
>
> #ifdef __KERNEL__
> #define KVM_MSR_FILTER_DEFAULT_ALLOW (0 << 0)
> #endif
>
> so that we don't try to use it in the kernel?  E.g. I can see someone doing
>
>         if (filter.flags & KVM_MSR_FILTER_DEFAULT_ALLOW)
>                 <allow the MSR>
>
> and getting really confused when that doesn't work.
>
> Or if we're feeling lucky, just remove it entirely as userspace doing
>
>         filter.flags &= KVM_MSR_FILTER_DEFAULT_ALLOW;
>
> is going to make someone sad someday.

Agreed that removing it would be more ideal, but I'm not feeling that
lucky to assume userspace isn't already using it.  I think it'll go
with your first option and wrap it in an #ifndef __KERNEL__.

>
> >  #define KVM_MSR_FILTER_DEFAULT_DENY  (1 << 0)
> > +#define KVM_MSR_FILTER_VALID_MASK (KVM_MSR_FILTER_DEFAULT_DENY)
> >       __u32 flags;
> >       struct kvm_msr_filter_range ranges[KVM_MSR_FILTER_MAX_RANGES];
> >  };