Re: [PATCH 2/2] KVM: TDX: Enable Bus Lock VM exit

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unmasked bus_lock_detected bit breaks TDX exit handling and guest execution.
- [High] Unvalidated VMCS write for BUS_LOCK_DETECTION allows userspace to trigger host WARN and crash VM.
- [High] tdx_handle_exit() silently masks negative error codes, returning undefined state to userspace.
--

--- Patch [2]: [PATCH 2/2] KVM: TDX: Enable Bus Lock VM exit ---
commit a95d478711dc8c7f94ecddd768bf45d636ab7635
Author: Xiaoyao Li <[email protected]>

KVM: TDX: Enable Bus Lock VM exit
    
Enable Bus Lock VM exit functionality for TDX guests by programming the
BUS_LOCK_DETECTION control in the TD VMCS and by adding the exit handler.

> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index cdc0d24657acb..c037e9cb5bdfc 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2031,7 +2031,7 @@ int tdx_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
>  }
>  
>  
> -int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> +static int __tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
>  {
>  	struct vcpu_tdx *tdx = to_tdx(vcpu);
>  	u64 vp_enter_ret = tdx->vp_enter_ret;

[Severity: High]
Does this code handle the bus_lock_detected bit (bit 26) being set in
vp_enter_ret or the raw 32-bit exit reason?

If userspace triggers a bus lock concurrently with an exit, the hardware can
set bit 26.  Will strict equality checks on vp_enter_ret (such as checking for
EXIT_REASON_EPT_MISCONFIG later in this function) fail because the bit is
unmasked?

This also appears to affect tdx_to_vmx_exit_reason(), where comparing the raw
exit reason directly against 16-bit constants like EXIT_REASON_TDCALL will
fail to match if the bus lock bit is set, leading to incorrect emulation.

[ ... ]

> @@ -2141,6 +2143,21 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
>  	return 0;
>  }
>  
> +int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> +{
> +	int ret = __tdx_handle_exit(vcpu, fastpath);
> +
> +	/* Exit to user space when bus lock was detected */
> +	if (vmx_get_exit_reason(vcpu).bus_lock_detected) {
> +		if (ret > 0)
> +			vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK;
> +
> +		vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK;
> +		return 0;

[Severity: High]
Does this code silently drop negative error codes?

If __tdx_handle_exit() returns a negative error code (like -EIO or -EFAULT)
while bus_lock_detected is true, it skips setting vcpu->run->exit_reason but
still returns 0.

Could this cause KVM to return success to userspace with an uninitialized or
stale exit_reason instead of propagating the error?

> +	}
> +	return ret;
> +}

[ ... ]

> @@ -3167,6 +3184,10 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
>  				vcpu->kvm->arch.notify_window);
>  	}
>  
> +	if (vcpu->kvm->arch.bus_lock_detection_enabled)
> +		td_vmcs_setbit32(tdx, SECONDARY_VM_EXEC_CONTROL,
> +				 SECONDARY_EXEC_BUS_LOCK_DETECTION);

[Severity: High]
Is it safe to write this execution control to the TD VMCS without verifying if
the TDX module supports it?

If a host userspace process enables KVM_CAP_X86_BUS_LOCK_EXIT but the TDX
module restricts this capability, could td_vmcs_setbit32() fail on TDH.VP.WR,
triggering the KVM_BUG_ON() macro and destroying the VM?

> +
>  	tdx->state = VCPU_TD_STATE_INITIALIZED;
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2