Re: [PATCH 1/2] KVM: nVMX: Don't clobber VM-Entry instruction length when saving soft INTn

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: nVMX: vm_exit_instruction_len overwritten in prepare_vmcs12()
--

commit 24bdb8dd4aa6675d8e53850c4d0acf71d5903e75
Author: Mahmoud Abumandour <ma.mandourr@gmail.com>

KVM: nVMX: Don't clobber VM-Entry instruction length when saving soft INTn

This patch updates vmcs12_save_pending_event() so that when a nested VM-Exit
occurs while vectoring a software interrupt, vcpu->arch.event_exit_inst_len is
written to vmcs12->vm_exit_instruction_len instead of
vmcs12->vm_entry_instruction_len.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 9b0bfa2f854cf..302f5b5aa5823 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4016,7 +4016,7 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
>  
>  		if (vcpu->arch.interrupt.soft) {
>  			idt_vectoring |= INTR_TYPE_SOFT_INTR;
> -			vmcs12->vm_entry_instruction_len =
> +			vmcs12->vm_exit_instruction_len =
>  				vcpu->arch.event_exit_inst_len;
>  		} else
>  			idt_vectoring |= INTR_TYPE_EXT_INTR;

[Severity: Medium]
Does writing vmcs12->vm_exit_instruction_len in vmcs12_save_pending_event()
actually take effect?

prepare_vmcs12() in arch/x86/kvm/vmx/nested.c is the sole caller of
vmcs12_save_pending_event(), and since commit c3634d25fbee ("KVM: nVMX: Leave
most VM-Exit info fields unmodified on failed VM-Entry"), it unconditionally
overwrites vmcs12->vm_exit_instruction_len with exit_insn_len right after
vmcs12_save_pending_event() returns:

arch/x86/kvm/vmx/nested.c:prepare_vmcs12() {
    ...
		vmcs12_save_pending_event(vcpu, vmcs12,
					  vm_exit_reason, exit_intr_info);

		vmcs12->vm_exit_intr_info = exit_intr_info;
		vmcs12->vm_exit_instruction_len = exit_insn_len;
		vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
    ...
}

This also affects the existing kvm_exception_is_soft(nr) branch in
vmcs12_save_pending_event():

arch/x86/kvm/vmx/nested.c:vmcs12_save_pending_event() {
    ...
		if (kvm_exception_is_soft(nr)) {
			vmcs12->vm_exit_instruction_len =
				vcpu->arch.event_exit_inst_len;
			idt_vectoring |= INTR_TYPE_SOFT_EXCEPTION;
		} else
    ...
}

When a nested VM-Exit is synthesized while a software interrupt or software
exception is being vectored and vcpu->arch.event_exit_inst_len differs from
exit_insn_len (for instance, after vmx_cancel_injection() or after state
restore via KVM_SET_NESTED_STATE and KVM_SET_VCPU_EVENTS prior to
kvm_check_nested_events() -> nested_vmx_inject_exception_vmexit()), won't
prepare_vmcs12() clobber vmcs12->vm_exit_instruction_len with a stale or zero
exit_insn_len?

Should prepare_vmcs12() assign vmcs12->vm_exit_instruction_len = exit_insn_len
before calling vmcs12_save_pending_event() so that soft interrupts and soft
exceptions can override it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021042603.608292-1-ma.mandourr@gmail.com?part=1