| Message ID | 20220719234950.3612318-2-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MSR filtering / exiting flag cleanup | expand |
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]; > };
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]; > > };
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 */
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(+)