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@xxxxxxxxxx> > --- > > 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 >
