| Message ID | 20220714161314.1715227-1-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] KVM: x86: Protect the unused bits in MSR exiting flags | expand |
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 >
On 7/14/22 18:13, Aaron Lewis wrote: > --- > > 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. For long term simplicity my hope is to be able to just patch > the original change. Yes, let's do it this way. Paolo
On Tue, Jul 19, 2022, Paolo Bonzini wrote: > On 7/14/22 18:13, Aaron Lewis wrote: > > --- > > > > 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. For long term simplicity my hope is to be able to just patch > > the original change. > > Yes, let's do it this way. Heh, which way is "this way"? (a) Fix KVM_CAP_X86_USER_SPACE_MSR and cross our fingers (b) Add a quirk (c) ???
On Tue, Jul 19, 2022 at 6:55 PM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Jul 19, 2022, Paolo Bonzini wrote: > > On 7/14/22 18:13, Aaron Lewis wrote: > > > --- > > > > > > 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. For long term simplicity my hope is to be able to just patch > > > the original change. > > > > Yes, let's do it this way. > > Heh, which way is "this way"? > > (a) Fix KVM_CAP_X86_USER_SPACE_MSR and cross our fingers > (b) Add a quirk > (c) ??? I hope we're talking about (a). I should have a patchset out later today with fixes and selftest.
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)) + 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;
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. For long term simplicity my hope is to be able to just patch the original change. arch/x86/kvm/x86.c | 8 ++++++++ 1 file changed, 8 insertions(+)