LinuxLists.cc - [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

2022-08-12 01:55:06

Subject: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

Add checking to VMCS12's "VMCS shadowing", make sure the checking of
VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
only if VMCS12's "VMCS shadowing" is 1.

SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
(and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
condition checking of [B] is reached(please refer [A]), which means
checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
happen only when "VMCS shadowing" = 1.

Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:

IF (not in VMX operation)
or (CR0.PE = 0)
or (RFLAGS.VM = 1)
or (IA32_EFER.LMA = 1 and CS.L = 0)
THEN #UD;
ELSIF in VMX non-root operation
AND (“VMCS shadowing” is 0 OR
source operand sets bits in range 63:15 OR
VMREAD bit corresponding to bits 14:0 of source
operand is 1) <------[A]
THEN VMexit;
ELSIF CPL > 0
THEN #GP(0);
ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
(in VMX non-root operation AND VMCS link pointer is not valid)
THEN VMfailInvalid; <------ [B]
...

Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
Signed-off-by: Yuan Yao <[email protected]>
---
arch/x86/kvm/vmx/nested.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ddd4367d4826..30685be54c5d 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
*/
if (vmx->nested.current_vmptr == INVALID_GPA ||
(is_guest_mode(vcpu) &&
+ nested_cpu_has_shadow_vmcs(vcpu) &&
get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
return nested_vmx_failInvalid(vcpu);

@@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
*/
if (vmx->nested.current_vmptr == INVALID_GPA ||
(is_guest_mode(vcpu) &&
+ nested_cpu_has_shadow_vmcs(vcpu) &&
get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
return nested_vmx_failInvalid(vcpu);

--
2.27.0

2022-08-12 02:33:56

by Yuan Yao

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

On Fri, Aug 12, 2022 at 09:47:06AM +0800, Yuan Yao wrote:
> Add checking to VMCS12's "VMCS shadowing", make sure the checking of
> VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
> only if VMCS12's "VMCS shadowing" is 1.
>
> SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
> (and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
> condition checking of [B] is reached(please refer [A]), which means
> checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
> happen only when "VMCS shadowing" = 1.
>
> Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:
>
> IF (not in VMX operation)
> or (CR0.PE = 0)
> or (RFLAGS.VM = 1)
> or (IA32_EFER.LMA = 1 and CS.L = 0)
> THEN #UD;
> ELSIF in VMX non-root operation
> AND (“VMCS shadowing” is 0 OR
> source operand sets bits in range 63:15 OR
> VMREAD bit corresponding to bits 14:0 of source
> operand is 1) <------[A]
> THEN VMexit;
> ELSIF CPL > 0
> THEN #GP(0);
> ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
> (in VMX non-root operation AND VMCS link pointer is not valid)
> THEN VMfailInvalid; <------ [B]
> ...
>
> Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
> Signed-off-by: Yuan Yao <[email protected]>
> ---
> arch/x86/kvm/vmx/nested.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ddd4367d4826..30685be54c5d 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
> */
> if (vmx->nested.current_vmptr == INVALID_GPA ||
> (is_guest_mode(vcpu) &&
> + nested_cpu_has_shadow_vmcs(vcpu) &&

Oops, should be "nested_cpu_has_shadow_vmcs(get_vmcs12(vcpu))".

> get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> return nested_vmx_failInvalid(vcpu);
>
> @@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> */
> if (vmx->nested.current_vmptr == INVALID_GPA ||
> (is_guest_mode(vcpu) &&
> + nested_cpu_has_shadow_vmcs(vcpu) &&

Ditto.

> get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> return nested_vmx_failInvalid(vcpu);
>
> --
> 2.27.0
>

2022-08-12 15:15:28

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

Hi Yuan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/queue]
[also build test ERROR on mst-vhost/linux-next linus/master v5.19 next-20220812]
[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#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Yuan-Yao/kvm-nVMX-Checks-VMCS-shadowing-with-VMCS-link-pointer-for-non-root-mode-VM-READ-WRITE/20220812-095001
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20220812/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
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/b15f3d4cd8e8f9cf2db64711234ca27ac74142b2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yuan-Yao/kvm-nVMX-Checks-VMCS-shadowing-with-VMCS-link-pointer-for-non-root-mode-VM-READ-WRITE/20220812-095001
git checkout b15f3d4cd8e8f9cf2db64711234ca27ac74142b2
# 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 <[email protected]>

All errors (new ones prefixed by >>):

>> arch/x86/kvm/vmx/nested.c:5126:35: error: incompatible pointer types passing 'struct kvm_vcpu *' to parameter of type 'struct vmcs12 *' [-Werror,-Wincompatible-pointer-types]
nested_cpu_has_shadow_vmcs(vcpu) &&
^~~~
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
arch/x86/kvm/vmx/nested.h:215:62: note: passing argument to parameter 'vmcs12' here
static inline bool nested_cpu_has_shadow_vmcs(struct vmcs12 *vmcs12)
^
>> arch/x86/kvm/vmx/nested.c:5126:35: error: incompatible pointer types passing 'struct kvm_vcpu *' to parameter of type 'struct vmcs12 *' [-Werror,-Wincompatible-pointer-types]
nested_cpu_has_shadow_vmcs(vcpu) &&
^~~~
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
arch/x86/kvm/vmx/nested.h:215:62: note: passing argument to parameter 'vmcs12' here
static inline bool nested_cpu_has_shadow_vmcs(struct vmcs12 *vmcs12)
^
>> arch/x86/kvm/vmx/nested.c:5126:35: error: incompatible pointer types passing 'struct kvm_vcpu *' to parameter of type 'struct vmcs12 *' [-Werror,-Wincompatible-pointer-types]
nested_cpu_has_shadow_vmcs(vcpu) &&
^~~~
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
arch/x86/kvm/vmx/nested.h:215:62: note: passing argument to parameter 'vmcs12' here
static inline bool nested_cpu_has_shadow_vmcs(struct vmcs12 *vmcs12)
^
arch/x86/kvm/vmx/nested.c:5237:34: error: incompatible pointer types passing 'struct kvm_vcpu *' to parameter of type 'struct vmcs12 *' [-Werror,-Wincompatible-pointer-types]
nested_cpu_has_shadow_vmcs(vcpu) &&
^~~~
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
arch/x86/kvm/vmx/nested.h:215:62: note: passing argument to parameter 'vmcs12' here
static inline bool nested_cpu_has_shadow_vmcs(struct vmcs12 *vmcs12)
^
arch/x86/kvm/vmx/nested.c:5237:34: error: incompatible pointer types passing 'struct kvm_vcpu *' to parameter of type 'struct vmcs12 *' [-Werror,-Wincompatible-pointer-types]
nested_cpu_has_shadow_vmcs(vcpu) &&
^~~~
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
arch/x86/kvm/vmx/nested.h:215:62: note: passing argument to parameter 'vmcs12' here
static inline bool nested_cpu_has_shadow_vmcs(struct vmcs12 *vmcs12)
^
arch/x86/kvm/vmx/nested.c:5237:34: error: incompatible pointer types passing 'struct kvm_vcpu *' to parameter of type 'struct vmcs12 *' [-Werror,-Wincompatible-pointer-types]
nested_cpu_has_shadow_vmcs(vcpu) &&
^~~~
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
arch/x86/kvm/vmx/nested.h:215:62: note: passing argument to parameter 'vmcs12' here
static inline bool nested_cpu_has_shadow_vmcs(struct vmcs12 *vmcs12)
^
6 errors generated.

vim +5126 arch/x86/kvm/vmx/nested.c

5098
5099 static int handle_vmread(struct kvm_vcpu *vcpu)
5100 {
5101 struct vmcs12 *vmcs12 = is_guest_mode(vcpu) ? get_shadow_vmcs12(vcpu)
5102 : get_vmcs12(vcpu);
5103 unsigned long exit_qualification = vmx_get_exit_qual(vcpu);
5104 u32 instr_info = vmcs_read32(VMX_INSTRUCTION_INFO);
5105 struct vcpu_vmx *vmx = to_vmx(vcpu);
5106 struct x86_exception e;
5107 unsigned long field;
5108 u64 value;
5109 gva_t gva = 0;
5110 short offset;
5111 int len, r;
5112
5113 if (!nested_vmx_check_permission(vcpu))
5114 return 1;
5115
5116 /* Decode instruction info and find the field to read */
5117 field = kvm_register_read(vcpu, (((instr_info) >> 28) & 0xf));
5118
5119 if (!evmptr_is_valid(vmx->nested.hv_evmcs_vmptr)) {
5120 /*
5121 * In VMX non-root operation, when the VMCS-link pointer is INVALID_GPA,
5122 * any VMREAD sets the ALU flags for VMfailInvalid.
5123 */
5124 if (vmx->nested.current_vmptr == INVALID_GPA ||
5125 (is_guest_mode(vcpu) &&
> 5126 nested_cpu_has_shadow_vmcs(vcpu) &&
5127 get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
5128 return nested_vmx_failInvalid(vcpu);
5129
5130 offset = get_vmcs12_field_offset(field);
5131 if (offset < 0)
5132 return nested_vmx_fail(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
5133
5134 if (!is_guest_mode(vcpu) && is_vmcs12_ext_field(field))
5135 copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
5136
5137 /* Read the field, zero-extended to a u64 value */
5138 value = vmcs12_read_any(vmcs12, field, offset);
5139 } else {
5140 /*
5141 * Hyper-V TLFS (as of 6.0b) explicitly states, that while an
5142 * enlightened VMCS is active VMREAD/VMWRITE instructions are
5143 * unsupported. Unfortunately, certain versions of Windows 11
5144 * don't comply with this requirement which is not enforced in
5145 * genuine Hyper-V. Allow VMREAD from an enlightened VMCS as a
5146 * workaround, as misbehaving guests will panic on VM-Fail.
5147 * Note, enlightened VMCS is incompatible with shadow VMCS so
5148 * all VMREADs from L2 should go to L1.
5149 */
5150 if (WARN_ON_ONCE(is_guest_mode(vcpu)))
5151 return nested_vmx_failInvalid(vcpu);
5152
5153 offset = evmcs_field_offset(field, NULL);
5154 if (offset < 0)
5155 return nested_vmx_fail(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
5156
5157 /* Read the field, zero-extended to a u64 value */
5158 value = evmcs_read_any(vmx->nested.hv_evmcs, field, offset);
5159 }
5160
5161 /*
5162 * Now copy part of this value to register or memory, as requested.
5163 * Note that the number of bits actually copied is 32 or 64 depending
5164 * on the guest's mode (32 or 64 bit), not on the given field's length.
5165 */
5166 if (instr_info & BIT(10)) {
5167 kvm_register_write(vcpu, (((instr_info) >> 3) & 0xf), value);
5168 } else {
5169 len = is_64_bit_mode(vcpu) ? 8 : 4;
5170 if (get_vmx_mem_address(vcpu, exit_qualification,
5171 instr_info, true, len, &gva))
5172 return 1;
5173 /* _system ok, nested_vmx_check_permission has verified cpl=0 */
5174 r = kvm_write_guest_virt_system(vcpu, gva, &value, len, &e);
5175 if (r != X86EMUL_CONTINUE)
5176 return kvm_handle_memory_failure(vcpu, r, &e);
5177 }
5178
5179 return nested_vmx_succeed(vcpu);
5180 }
5181

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-08-12 15:41:41

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

2022-08-12 19:32:26

by Jim Mattson

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

On Thu, Aug 11, 2022 at 7:02 PM Yuan Yao <[email protected]> wrote:
>
> On Fri, Aug 12, 2022 at 09:47:06AM +0800, Yuan Yao wrote:
> > Add checking to VMCS12's "VMCS shadowing", make sure the checking of
> > VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
> > only if VMCS12's "VMCS shadowing" is 1.
> >
> > SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
> > (and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
> > condition checking of [B] is reached(please refer [A]), which means
> > checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
> > happen only when "VMCS shadowing" = 1.
> >
> > Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:
> >
> > IF (not in VMX operation)
> > or (CR0.PE = 0)
> > or (RFLAGS.VM = 1)
> > or (IA32_EFER.LMA = 1 and CS.L = 0)
> > THEN #UD;
> > ELSIF in VMX non-root operation
> > AND (“VMCS shadowing” is 0 OR
> > source operand sets bits in range 63:15 OR
> > VMREAD bit corresponding to bits 14:0 of source
> > operand is 1) <------[A]
> > THEN VMexit;
> > ELSIF CPL > 0
> > THEN #GP(0);
> > ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
> > (in VMX non-root operation AND VMCS link pointer is not valid)
> > THEN VMfailInvalid; <------ [B]
> > ...
> >
> > Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
> > Signed-off-by: Yuan Yao <[email protected]>
> > ---
> > arch/x86/kvm/vmx/nested.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index ddd4367d4826..30685be54c5d 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
> > */
> > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > (is_guest_mode(vcpu) &&
> > + nested_cpu_has_shadow_vmcs(vcpu) &&
>
> Oops, should be "nested_cpu_has_shadow_vmcs(get_vmcs12(vcpu))".
>
> > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > return nested_vmx_failInvalid(vcpu);
> >
> > @@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> > */
> > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > (is_guest_mode(vcpu) &&
> > + nested_cpu_has_shadow_vmcs(vcpu) &&
>
> Ditto.
>
> > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > return nested_vmx_failInvalid(vcpu);
> >
> > --
> > 2.27.0
> >

2022-08-12 20:11:44

by Jim Mattson

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

2022-08-12 23:12:39

by Yao Yuan

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

On Fri, Aug 12, 2022 at 12:33:05PM -0700, Jim Mattson wrote:
> On Thu, Aug 11, 2022 at 7:02 PM Yuan Yao <[email protected]> wrote:
> >
> > On Fri, Aug 12, 2022 at 09:47:06AM +0800, Yuan Yao wrote:
> > > Add checking to VMCS12's "VMCS shadowing", make sure the checking of
> > > VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
> > > only if VMCS12's "VMCS shadowing" is 1.
> > >
> > > SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
> > > (and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
> > > condition checking of [B] is reached(please refer [A]), which means
> > > checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
> > > happen only when "VMCS shadowing" = 1.
> > >
> > > Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:
> > >
> > > IF (not in VMX operation)
> > > or (CR0.PE = 0)
> > > or (RFLAGS.VM = 1)
> > > or (IA32_EFER.LMA = 1 and CS.L = 0)
> > > THEN #UD;
> > > ELSIF in VMX non-root operation
> > > AND (“VMCS shadowing” is 0 OR
> > > source operand sets bits in range 63:15 OR
> > > VMREAD bit corresponding to bits 14:0 of source
> > > operand is 1) <------[A]
> > > THEN VMexit;
> > > ELSIF CPL > 0
> > > THEN #GP(0);
> > > ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
> > > (in VMX non-root operation AND VMCS link pointer is not valid)
> > > THEN VMfailInvalid; <------ [B]
> > > ...
> > >
> > > Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
> > > Signed-off-by: Yuan Yao <[email protected]>
> > > ---
> > > arch/x86/kvm/vmx/nested.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index ddd4367d4826..30685be54c5d 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
> > > */
> > > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > > (is_guest_mode(vcpu) &&
> > > + nested_cpu_has_shadow_vmcs(vcpu) &&
> >
> > Oops, should be "nested_cpu_has_shadow_vmcs(get_vmcs12(vcpu))".
> >
> > > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > > return nested_vmx_failInvalid(vcpu);
> > >
> > > @@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> > > */
> > > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > > (is_guest_mode(vcpu) &&
> > > + nested_cpu_has_shadow_vmcs(vcpu) &&
> >
> > Ditto.
> >
> > > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > > return nested_vmx_failInvalid(vcpu);
> > >
> > > --
>
> These checks are redundant, aren't they?
>
> That is, nested_vmx_exit_handled_vmcs_access() has already checked
> nested_cpu_has_shadow_vmcs(vmcs12).

Ah, you're right it does there.

That means in L0 we handle this for vmcs12 which has shadow VMCS
setting and the corresponding bit in the bitmap is 0(so no vmexit to
L1 and the read/write should from/to vmcs12's shadow vmcs, we handle
this here to emulate this), so we don't need to check the shdaow VMCS
setting here again. Is this the right understanding ?

2022-08-13 01:02:41

by Jim Mattson

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

On Fri, Aug 12, 2022 at 4:08 PM Yao Yuan <[email protected]> wrote:
>
> On Fri, Aug 12, 2022 at 12:33:05PM -0700, Jim Mattson wrote:
> > On Thu, Aug 11, 2022 at 7:02 PM Yuan Yao <[email protected]> wrote:
> > >
> > > On Fri, Aug 12, 2022 at 09:47:06AM +0800, Yuan Yao wrote:
> > > > Add checking to VMCS12's "VMCS shadowing", make sure the checking of
> > > > VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
> > > > only if VMCS12's "VMCS shadowing" is 1.
> > > >
> > > > SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
> > > > (and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
> > > > condition checking of [B] is reached(please refer [A]), which means
> > > > checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
> > > > happen only when "VMCS shadowing" = 1.
> > > >
> > > > Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:
> > > >
> > > > IF (not in VMX operation)
> > > > or (CR0.PE = 0)
> > > > or (RFLAGS.VM = 1)
> > > > or (IA32_EFER.LMA = 1 and CS.L = 0)
> > > > THEN #UD;
> > > > ELSIF in VMX non-root operation
> > > > AND (“VMCS shadowing” is 0 OR
> > > > source operand sets bits in range 63:15 OR
> > > > VMREAD bit corresponding to bits 14:0 of source
> > > > operand is 1) <------[A]
> > > > THEN VMexit;
> > > > ELSIF CPL > 0
> > > > THEN #GP(0);
> > > > ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
> > > > (in VMX non-root operation AND VMCS link pointer is not valid)
> > > > THEN VMfailInvalid; <------ [B]
> > > > ...
> > > >
> > > > Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
> > > > Signed-off-by: Yuan Yao <[email protected]>
> > > > ---
> > > > arch/x86/kvm/vmx/nested.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > > index ddd4367d4826..30685be54c5d 100644
> > > > --- a/arch/x86/kvm/vmx/nested.c
> > > > +++ b/arch/x86/kvm/vmx/nested.c
> > > > @@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
> > > > */
> > > > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > > > (is_guest_mode(vcpu) &&
> > > > + nested_cpu_has_shadow_vmcs(vcpu) &&
> > >
> > > Oops, should be "nested_cpu_has_shadow_vmcs(get_vmcs12(vcpu))".
> > >
> > > > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > > > return nested_vmx_failInvalid(vcpu);
> > > >
> > > > @@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> > > > */
> > > > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > > > (is_guest_mode(vcpu) &&
> > > > + nested_cpu_has_shadow_vmcs(vcpu) &&
> > >
> > > Ditto.
> > >
> > > > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > > > return nested_vmx_failInvalid(vcpu);
> > > >
> > > > --
> >
> > These checks are redundant, aren't they?
> >
> > That is, nested_vmx_exit_handled_vmcs_access() has already checked
> > nested_cpu_has_shadow_vmcs(vmcs12).
>
> Ah, you're right it does there.
>
> That means in L0 we handle this for vmcs12 which has shadow VMCS
> setting and the corresponding bit in the bitmap is 0(so no vmexit to
> L1 and the read/write should from/to vmcs12's shadow vmcs, we handle
> this here to emulate this), so we don't need to check the shdaow VMCS
> setting here again. Is this the right understanding ?

That is correct.

2022-08-14 11:22:09

by Yao Yuan

[permalink] [raw]

Subject: Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

On Fri, Aug 12, 2022 at 05:30:53PM -0700, Jim Mattson wrote:
> On Fri, Aug 12, 2022 at 4:08 PM Yao Yuan <[email protected]> wrote:
> >
> > On Fri, Aug 12, 2022 at 12:33:05PM -0700, Jim Mattson wrote:
> > > On Thu, Aug 11, 2022 at 7:02 PM Yuan Yao <[email protected]> wrote:
> > > >
> > > > On Fri, Aug 12, 2022 at 09:47:06AM +0800, Yuan Yao wrote:
> > > > > Add checking to VMCS12's "VMCS shadowing", make sure the checking of
> > > > > VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
> > > > > only if VMCS12's "VMCS shadowing" is 1.
> > > > >
> > > > > SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
> > > > > (and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
> > > > > condition checking of [B] is reached(please refer [A]), which means
> > > > > checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
> > > > > happen only when "VMCS shadowing" = 1.
> > > > >
> > > > > Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:
> > > > >
> > > > > IF (not in VMX operation)
> > > > > or (CR0.PE = 0)
> > > > > or (RFLAGS.VM = 1)
> > > > > or (IA32_EFER.LMA = 1 and CS.L = 0)
> > > > > THEN #UD;
> > > > > ELSIF in VMX non-root operation
> > > > > AND (“VMCS shadowing” is 0 OR
> > > > > source operand sets bits in range 63:15 OR
> > > > > VMREAD bit corresponding to bits 14:0 of source
> > > > > operand is 1) <------[A]
> > > > > THEN VMexit;
> > > > > ELSIF CPL > 0
> > > > > THEN #GP(0);
> > > > > ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
> > > > > (in VMX non-root operation AND VMCS link pointer is not valid)
> > > > > THEN VMfailInvalid; <------ [B]
> > > > > ...
> > > > >
> > > > > Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
> > > > > Signed-off-by: Yuan Yao <[email protected]>
> > > > > ---
> > > > > arch/x86/kvm/vmx/nested.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > > > index ddd4367d4826..30685be54c5d 100644
> > > > > --- a/arch/x86/kvm/vmx/nested.c
> > > > > +++ b/arch/x86/kvm/vmx/nested.c
> > > > > @@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
> > > > > */
> > > > > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > > > > (is_guest_mode(vcpu) &&
> > > > > + nested_cpu_has_shadow_vmcs(vcpu) &&
> > > >
> > > > Oops, should be "nested_cpu_has_shadow_vmcs(get_vmcs12(vcpu))".
> > > >
> > > > > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > > > > return nested_vmx_failInvalid(vcpu);
> > > > >
> > > > > @@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> > > > > */
> > > > > if (vmx->nested.current_vmptr == INVALID_GPA ||
> > > > > (is_guest_mode(vcpu) &&
> > > > > + nested_cpu_has_shadow_vmcs(vcpu) &&
> > > >
> > > > Ditto.
> > > >
> > > > > get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > > > > return nested_vmx_failInvalid(vcpu);
> > > > >
> > > > > --
> > >
> > > These checks are redundant, aren't they?
> > >
> > > That is, nested_vmx_exit_handled_vmcs_access() has already checked
> > > nested_cpu_has_shadow_vmcs(vmcs12).
> >
> > Ah, you're right it does there.
> >
> > That means in L0 we handle this for vmcs12 which has shadow VMCS
> > setting and the corresponding bit in the bitmap is 0(so no vmexit to
> > L1 and the read/write should from/to vmcs12's shadow vmcs, we handle
> > this here to emulate this), so we don't need to check the shdaow VMCS
> > setting here again. Is this the right understanding ?
>
> That is correct.

I got it, Thanks a lot for your correction!