Re: [RFC PATCH] KVM: x86: Protect the unused bits in MSR exiting flags

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 14, 2022, Aaron Lewis wrote:
> The flags for 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>
> ---
> 
> 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.
> 
> Another option would be to correct this by adding a quirk, but fixing
> it that has its down sides.   It complicates the code more than it
> would otherwise be, and complicates the usage for anyone using any new
> features introduce in the future because they would also have to enable
> a quirk.

The tried and true KVM method of adding '2' is probably the way to go:

	case KVM_CAP_X86_USER_SPACE_MSR2:
		r = -EINVAL;
		if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK))
			break;
		fallthrough;
	case KVM_CAP_X86_USER_SPACE_MSR:
		if (cap->cap == KVM_CAP_X86_USER_SPACE_MSR)
			cap->args[0] &= KVM_MSR_EXIT_REASON_VALID_V1_MASK;
		kvm->arch.user_space_msr_mask = cap->args[0];
		r = 0;
		break;

Or maybe

	case KVM_CAP_X86_USER_SPACE_MSR2:
	case KVM_CAP_X86_USER_SPACE_MSR:
		r = -EINVAL;
		if (cap->cap == KVM_CAP_X86_USER_SPACE_MSR)
			cap->args[0] &= KVM_MSR_EXIT_REASON_VALID_V1_MASK;
		else if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK))
			break;
		kvm->arch.user_space_msr_mask = cap->args[0];
		r = 0;
		break;

Ugly, but not too complex.  But I'm getting ahead of things :-)

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

Agreed.

>  arch/x86/kvm/x86.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1910e1e78b15..ae9b7df86b1a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6029,6 +6029,11 @@ 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_INVAL |
> +				     KVM_MSR_EXIT_REASON_UNKNOWN |
> +				     KVM_MSR_EXIT_REASON_FILTER))

Add a KVM_MSR_EXIT_REASON_VALID_MASK to define all of these.  Ditto for the
filter flags even though there's only one.  And if we want to go this route, we
shouldn't definitely add a testcase in selftests.

> +			break;
>  		kvm->arch.user_space_msr_mask = cap->args[0];
>  		r = 0;
>  		break;
> @@ -6183,6 +6188,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_DEFAULT_DENY)
> +		return -EINVAL;
> +
>  	for (i = 0; i < ARRAY_SIZE(filter.ranges); i++)
>  		empty &= !filter.ranges[i].nmsrs;
>  
> -- 
> 2.37.0.144.g8ac04bfd2-goog
>