Re: [PATCH v2] KVM: nVMX: Don't leak L1 MMIO regions to L2

[kbuild test robot]

CC: kbuil...@01.org
In-Reply-To: <20191009000343....@google.com>
References: <20191009000343....@google.com>
TO: Jim Mattson <jmat...@google.com>
CC: k...@vger.kernel.org, Liran Alon <liran...@oracle.com>, Sean Christopherson <sean.j.chr...@intel.com>, Paolo Bonzini <pbon...@redhat.com>, Jim Mattson <jmat...@google.com>, Dan Cross <dcr...@google.com>, Peter Shier <psh...@google.com>, Jim Mattson <jmat...@google.com>, Dan Cross <dcr...@google.com>, Peter Shier <psh...@google.com>
CC: Jim Mattson <jmat...@google.com>, Dan Cross <dcr...@google.com>, Peter Shier <psh...@google.com>

Hi Jim,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kvm/linux-next]
[cannot apply to v5.4-rc2 next-20191008]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Jim-Mattson/KVM-nVMX-Don-t-leak-L1-MMIO-regions-to-L2/20191009-120808
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-rhel-7.6 (attached as .config)
compiler: clang version 10.0.0 (git://gitmirror/llvm_project 41c934acaf8539dedad4b48bbc88580c74fed25a)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <l...@intel.com>

All warnings (new ones prefixed by >>):

>> arch/x86/kvm/vmx/nested.c:3253:11: warning: enumeration value 'ENTER_VMX_SUCCESS' not handled in switch [-Wswitch]
switch (status) {
^
1 warning generated.

vim +/ENTER_VMX_SUCCESS +3253 arch/x86/kvm/vmx/nested.c

3180
3181 /*
3182 * nested_vmx_run() handles a nested entry, i.e., a VMLAUNCH or VMRESUME on L1
3183 * for running an L2 nested guest.
3184 */
3185 static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
3186 {
3187 struct vmcs12 *vmcs12;
3188 enum enter_vmx_status status;
3189 struct vcpu_vmx *vmx = to_vmx(vcpu);
3190 u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
3191
3192 if (!nested_vmx_check_permission(vcpu))
3193 return 1;
3194
3195 if (!nested_vmx_handle_enlightened_vmptrld(vcpu, launch))
3196 return 1;
3197
3198 if (!vmx->nested.hv_evmcs && vmx->nested.current_vmptr == -1ull)
3199 return nested_vmx_failInvalid(vcpu);
3200
3201 vmcs12 = get_vmcs12(vcpu);
3202
3203 /*
3204 * Can't VMLAUNCH or VMRESUME a shadow VMCS. Despite the fact
3205 * that there *is* a valid VMCS pointer, RFLAGS.CF is set
3206 * rather than RFLAGS.ZF, and no error number is stored to the
3207 * VM-instruction error field.
3208 */
3209 if (vmcs12->hdr.shadow_vmcs)
3210 return nested_vmx_failInvalid(vcpu);
3211
3212 if (vmx->nested.hv_evmcs) {
3213 copy_enlightened_to_vmcs12(vmx);
3214 /* Enlightened VMCS doesn't have launch state */
3215 vmcs12->launch_state = !launch;
3216 } else if (enable_shadow_vmcs) {
3217 copy_shadow_to_vmcs12(vmx);
3218 }
3219
3220 /*
3221 * The nested entry process starts with enforcing various prerequisites
3222 * on vmcs12 as required by the Intel SDM, and act appropriately when
3223 * they fail: As the SDM explains, some conditions should cause the
3224 * instruction to fail, while others will cause the instruction to seem
3225 * to succeed, but return an EXIT_REASON_INVALID_STATE.
3226 * To speed up the normal (success) code path, we should avoid checking
3227 * for misconfigurations which will anyway be caught by the processor
3228 * when using the merged vmcs02.
3229 */
3230 if (interrupt_shadow & KVM_X86_SHADOW_INT_MOV_SS)
3231 return nested_vmx_failValid(vcpu,
3232 VMXERR_ENTRY_EVENTS_BLOCKED_BY_MOV_SS);
3233
3234 if (vmcs12->launch_state == launch)
3235 return nested_vmx_failValid(vcpu,
3236 launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
3237 : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
3238
3239 if (nested_vmx_check_controls(vcpu, vmcs12))
3240 return nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD);
3241
3242 if (nested_vmx_check_host_state(vcpu, vmcs12))
3243 return nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
3244
3245 /*
3246 * We're finally done with prerequisite checking, and can start with
3247 * the nested entry.
3248 */
3249 vmx->nested.nested_run_pending = 1;
3250 status = nested_vmx_enter_non_root_mode(vcpu, true);
3251 if (status != ENTER_VMX_SUCCESS) {
3252 vmx->nested.nested_run_pending = 0;
> 3253 switch (status) {
3254 case ENTER_VMX_VMFAIL:
3255 return nested_vmx_failValid(vcpu,
3256 VMXERR_ENTRY_INVALID_CONTROL_FIELD);
3257 case ENTER_VMX_VMEXIT:
3258 return 1;
3259 case ENTER_VMX_ERROR:
3260 return 0;
3261 }
3262 }
3263
3264 /* Hide L1D cache contents from the nested guest. */
3265 vmx->vcpu.arch.l1tf_flush_l1d = true;
3266
3267 /*
3268 * Must happen outside of nested_vmx_enter_non_root_mode() as it will
3269 * also be used as part of restoring nVMX state for
3270 * snapshot restore (migration).
3271 *
3272 * In this flow, it is assumed that vmcs12 cache was
3273 * trasferred as part of captured nVMX state and should
3274 * therefore not be read from guest memory (which may not
3275 * exist on destination host yet).
3276 */
3277 nested_cache_shadow_vmcs12(vcpu, vmcs12);
3278
3279 /*
3280 * If we're entering a halted L2 vcpu and the L2 vcpu won't be
3281 * awakened by event injection or by an NMI-window VM-exit or
3282 * by an interrupt-window VM-exit, halt the vcpu.
3283 */
3284 if ((vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT) &&
3285 !(vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK) &&
3286 !(vmcs12->cpu_based_vm_exec_control & CPU_BASED_VIRTUAL_NMI_PENDING) &&
3287 !((vmcs12->cpu_based_vm_exec_control & CPU_BASED_VIRTUAL_INTR_PENDING) &&
3288 (vmcs12->guest_rflags & X86_EFLAGS_IF))) {
3289 vmx->nested.nested_run_pending = 0;
3290 return kvm_vcpu_halt(vcpu);
3291 }
3292 return 1;
3293 }
3294

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

[Nathan Chancellor]

While the warning is not technically wrong, we cannot reach this switch
statement if status == ENTER_VMX_SUCCESS. I feel like this diagnostic
could be improved. I guess a default statement could be added
regardless...

https://godbolt.org/z/kEfe8Z

Cheers,
Nathan