On Wed, Jul 20, 2022 at 11:31 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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];
> > };
