diff mbox series

[3/3] KVM: x86: Don't deflect MSRs to userspace that can't be filtered

Message ID 20220703191636.2159067-4-aaronlewis@google.com (mailing list archive)
State New, archived
Headers show
Series MSR Filtering updates | expand

Commit Message

Aaron Lewis July 3, 2022, 7:16 p.m. UTC 
If an MSR is not permitted to be filtered and deflected to userspace,
don't then allow it to be deflected to userspace by other means.  If an
MSR that cannot be filtered #GP's, and KVM is configured to send all
MSRs that #GP to userspace, that MSR will be sent to userspace as well.
Prevent that from happening by filtering out disallowed MSRs from being
deflected to userspace.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/x86.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

kernel test robot July 3, 2022, 8:34 p.m. UTC | #1 
Hi Aaron,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kvm/queue]
[also build test WARNING on next-20220701]
[cannot apply to mst-vhost/linux-next linus/master v5.19-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aaron-Lewis/MSR-Filtering-updates/20220704-031727
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220704/202207040455.aag6l8Io-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/0c12a0d47fb511592df45bf2030cc200b5bab5ef
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aaron-Lewis/MSR-Filtering-updates/20220704-031727
        git checkout 0c12a0d47fb511592df45bf2030cc200b5bab5ef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/kvm/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/x86/kvm/x86.c:1715:6: warning: no previous prototype for 'kvm_msr_filtering_disallowed' [-Wmissing-prototypes]
    1715 | bool kvm_msr_filtering_disallowed(u32 index)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/kvm_msr_filtering_disallowed +1715 arch/x86/kvm/x86.c

  1714	
> 1715	bool kvm_msr_filtering_disallowed(u32 index)
  1716	{
  1717		/* x2APIC MSRs do not support filtering. */
  1718		if (index >= 0x800 && index <= 0x8ff)
  1719			return true;
  1720	
  1721		return false;
  1722	}
  1723
kernel test robot July 3, 2022, 9:05 p.m. UTC | #2 
Hi Aaron,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kvm/queue]
[also build test WARNING on next-20220701]
[cannot apply to mst-vhost/linux-next linus/master v5.19-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aaron-Lewis/MSR-Filtering-updates/20220704-031727
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20220704/202207040448.69LBG38o-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5d787689b14574fe58ba9798563f4a6df6059fbf)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/0c12a0d47fb511592df45bf2030cc200b5bab5ef
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aaron-Lewis/MSR-Filtering-updates/20220704-031727
        git checkout 0c12a0d47fb511592df45bf2030cc200b5bab5ef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/kvm/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/x86/kvm/x86.c:1715:6: warning: no previous prototype for function 'kvm_msr_filtering_disallowed' [-Wmissing-prototypes]
   bool kvm_msr_filtering_disallowed(u32 index)
        ^
   arch/x86/kvm/x86.c:1715:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   bool kvm_msr_filtering_disallowed(u32 index)
   ^
   static 
   1 warning generated.


vim +/kvm_msr_filtering_disallowed +1715 arch/x86/kvm/x86.c

  1714	
> 1715	bool kvm_msr_filtering_disallowed(u32 index)
  1716	{
  1717		/* x2APIC MSRs do not support filtering. */
  1718		if (index >= 0x800 && index <= 0x8ff)
  1719			return true;
  1720	
  1721		return false;
  1722	}
  1723
kernel test robot July 4, 2022, 12:09 a.m. UTC | #3 
Hi Aaron,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/queue]
[also build test ERROR on next-20220701]
[cannot apply to mst-vhost/linux-next linus/master v5.19-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aaron-Lewis/MSR-Filtering-updates/20220704-031727
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
config: i386-randconfig-a002 (https://download.01.org/0day-ci/archive/20220704/202207040818.udTt0mLB-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5d787689b14574fe58ba9798563f4a6df6059fbf)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/0c12a0d47fb511592df45bf2030cc200b5bab5ef
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aaron-Lewis/MSR-Filtering-updates/20220704-031727
        git checkout 0c12a0d47fb511592df45bf2030cc200b5bab5ef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> arch/x86/kvm/x86.c:1715:6: error: no previous prototype for function 'kvm_msr_filtering_disallowed' [-Werror,-Wmissing-prototypes]
   bool kvm_msr_filtering_disallowed(u32 index)
        ^
   arch/x86/kvm/x86.c:1715:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   bool kvm_msr_filtering_disallowed(u32 index)
   ^
   static 
   1 error generated.


vim +/kvm_msr_filtering_disallowed +1715 arch/x86/kvm/x86.c

  1714	
> 1715	bool kvm_msr_filtering_disallowed(u32 index)
  1716	{
  1717		/* x2APIC MSRs do not support filtering. */
  1718		if (index >= 0x800 && index <= 0x8ff)
  1719			return true;
  1720	
  1721		return false;
  1722	}
  1723
Sean Christopherson July 7, 2022, 7:21 p.m. UTC | #4 
On Sun, Jul 03, 2022, Aaron Lewis wrote:
> If an MSR is not permitted to be filtered and deflected to userspace,
> don't then allow it to be deflected to userspace by other means.  If an
> MSR that cannot be filtered #GP's, and KVM is configured to send all
> MSRs that #GP to userspace, that MSR will be sent to userspace as well.
> Prevent that from happening by filtering out disallowed MSRs from being
> deflected to userspace.

Why?  Honest question.  KVM doesn't allow filtering x2APIC accesses because
supporting that would be messy, and there's no sane use case for intercepting
x2APIC accesses if userspace has enabled the in-kernel local APIC.

I can't think of a meaningful use case for intercepting faults on x2APIC MSRs,
but I also don't see anything inherently broken with allowing userspace to intercept
such faults.

> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/x86.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 031678eff28e..a84741f7d254 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1712,6 +1712,15 @@ void kvm_enable_efer_bits(u64 mask)
>  }
>  EXPORT_SYMBOL_GPL(kvm_enable_efer_bits);
>  
> +bool kvm_msr_filtering_disallowed(u32 index)

Should be static, per the test bot.

> +{
> +	/* x2APIC MSRs do not support filtering. */
> +	if (index >= 0x800 && index <= 0x8ff)
> +		return true;
> +
> +	return false;
> +}
> +
>  bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
>  {
>  	struct kvm_x86_msr_filter *msr_filter;
> @@ -1721,8 +1730,8 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
>  	int idx;
>  	u32 i;
>  
> -	/* x2APIC MSRs do not support filtering. */
> -	if (index >= 0x800 && index <= 0x8ff)
> +	/* Prevent certain MSRs from using MSR Filtering. */
> +	if (kvm_msr_filtering_disallowed(index))
>  		return true;
>  
>  	idx = srcu_read_lock(&kvm->srcu);
> @@ -1962,6 +1971,9 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
>  	if (!(vcpu->kvm->arch.user_space_msr_mask & msr_reason))
>  		return 0;
>  
> +	if (kvm_msr_filtering_disallowed(index))
> +		return 0;
> +
>  	vcpu->run->exit_reason = exit_reason;
>  	vcpu->run->msr.error = 0;
>  	memset(vcpu->run->msr.pad, 0, sizeof(vcpu->run->msr.pad));
> -- 
> 2.37.0.rc0.161.g10f37bed90-goog
>
Aaron Lewis July 9, 2022, 2:05 a.m. UTC | #5 
On Thu, Jul 7, 2022 at 7:21 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Sun, Jul 03, 2022, Aaron Lewis wrote:
> > If an MSR is not permitted to be filtered and deflected to userspace,
> > don't then allow it to be deflected to userspace by other means.  If an
> > MSR that cannot be filtered #GP's, and KVM is configured to send all
> > MSRs that #GP to userspace, that MSR will be sent to userspace as well.
> > Prevent that from happening by filtering out disallowed MSRs from being
> > deflected to userspace.
>
> Why?  Honest question.  KVM doesn't allow filtering x2APIC accesses because
> supporting that would be messy, and there's no sane use case for intercepting
> x2APIC accesses if userspace has enabled the in-kernel local APIC.
>
> I can't think of a meaningful use case for intercepting faults on x2APIC MSRs,
> but I also don't see anything inherently broken with allowing userspace to intercept
> such faults.

Ack.  I'll drop it in v2.

>
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> >  arch/x86/kvm/x86.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 031678eff28e..a84741f7d254 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1712,6 +1712,15 @@ void kvm_enable_efer_bits(u64 mask)
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_enable_efer_bits);
> >
> > +bool kvm_msr_filtering_disallowed(u32 index)
>
> Should be static, per the test bot.
>
> > +{
> > +     /* x2APIC MSRs do not support filtering. */
> > +     if (index >= 0x800 && index <= 0x8ff)
> > +             return true;
> > +
> > +     return false;
> > +}
> > +
> >  bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
> >  {
> >       struct kvm_x86_msr_filter *msr_filter;
> > @@ -1721,8 +1730,8 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
> >       int idx;
> >       u32 i;
> >
> > -     /* x2APIC MSRs do not support filtering. */
> > -     if (index >= 0x800 && index <= 0x8ff)
> > +     /* Prevent certain MSRs from using MSR Filtering. */
> > +     if (kvm_msr_filtering_disallowed(index))
> >               return true;
> >
> >       idx = srcu_read_lock(&kvm->srcu);
> > @@ -1962,6 +1971,9 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
> >       if (!(vcpu->kvm->arch.user_space_msr_mask & msr_reason))
> >               return 0;
> >
> > +     if (kvm_msr_filtering_disallowed(index))
> > +             return 0;
> > +
> >       vcpu->run->exit_reason = exit_reason;
> >       vcpu->run->msr.error = 0;
> >       memset(vcpu->run->msr.pad, 0, sizeof(vcpu->run->msr.pad));
> > --
> > 2.37.0.rc0.161.g10f37bed90-goog
> >
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 031678eff28e..a84741f7d254 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1712,6 +1712,15 @@  void kvm_enable_efer_bits(u64 mask)
 }
 EXPORT_SYMBOL_GPL(kvm_enable_efer_bits);
 
+bool kvm_msr_filtering_disallowed(u32 index)
+{
+	/* x2APIC MSRs do not support filtering. */
+	if (index >= 0x800 && index <= 0x8ff)
+		return true;
+
+	return false;
+}
+
 bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
 {
 	struct kvm_x86_msr_filter *msr_filter;
@@ -1721,8 +1730,8 @@  bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
 	int idx;
 	u32 i;
 
-	/* x2APIC MSRs do not support filtering. */
-	if (index >= 0x800 && index <= 0x8ff)
+	/* Prevent certain MSRs from using MSR Filtering. */
+	if (kvm_msr_filtering_disallowed(index))
 		return true;
 
 	idx = srcu_read_lock(&kvm->srcu);
@@ -1962,6 +1971,9 @@  static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
 	if (!(vcpu->kvm->arch.user_space_msr_mask & msr_reason))
 		return 0;
 
+	if (kvm_msr_filtering_disallowed(index))
+		return 0;
+
 	vcpu->run->exit_reason = exit_reason;
 	vcpu->run->msr.error = 0;
 	memset(vcpu->run->msr.pad, 0, sizeof(vcpu->run->msr.pad));