kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Xiaoyao Li <xiaoyao.li@intel.com>
To: sashiko-reviews@lists.linux.dev
Cc: kvm@vger.kernel.org
Subject: Re: [PATCH 2/2] KVM: TDX: Enable Bus Lock VM exit
Date: Wed, 5 Aug 2026 15:53:31 +0800	[thread overview]
Message-ID: <4fedcaa6-d710-48f8-84c4-de682350bb2f@intel.com> (raw)
In-Reply-To: <20260805034602.5B2BB1F000E9@smtp.kernel.org>

On 8/5/2026 11:46 AM, sashiko-bot@kernel.org wrote:
>> 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?

Sashiko is correct. I think the handling like

   (vp_enter_ret == EXIT_REASON_EPT_MISCONFIG)

is not safe, we need to first mask vp_enter_ret. Will add a separate 
patch for it.

> 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.

This is valid. We need to adjust tdx_to_vmx_exit_reason().

However, there is a more important problem. Since Bus Lock VM exit makes 
bit 26 possible in EXIT REASON, the trick of "return -1" in 
tdx_to_vmx_exit_reason() will introduce false-positive in the following 
check of if(vmx_get_exit_reason(vcpu).bus_lock_detected) added by this 
patch.

how about something like below:

---8<---
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index c037e9cb5bdf..54598dcda938 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -927,7 +927,7 @@ static __always_inline u32 
tdcall_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
  static __always_inline u32 tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
  {
         struct vcpu_tdx *tdx = to_tdx(vcpu);
-       u32 exit_reason;
+       union vmx_exit_reason exit_reason;

         switch (tdx->vp_enter_ret & TDX_SEAMCALL_STATUS_MASK) {
         case TDX_SUCCESS:
@@ -937,28 +937,29 @@ static __always_inline u32 
tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
         case TDX_NON_RECOVERABLE_TD_WRONG_APIC_MODE:
                 break;
         default:
-               return -1u;
+               return (u16)-1u;
         }

-       exit_reason = tdx->vp_enter_ret;
+       exit_reason.full = tdx->vp_enter_ret;

-       switch (exit_reason) {
+       switch (exit_reason.basic) {
         case EXIT_REASON_TDCALL:
                 if (tdvmcall_exit_type(vcpu))
-                       return EXIT_REASON_VMCALL;
+                       exit_reason.basic = EXIT_REASON_VMCALL;

-               return tdcall_to_vmx_exit_reason(vcpu);
+               exit_reason.basic = tdcall_to_vmx_exit_reason(vcpu);
+               break;
         case EXIT_REASON_EPT_MISCONFIG:
                 /*
                  * Defer KVM_BUG_ON() until tdx_handle_exit() because 
this is in
                  * non-instrumentable code with interrupts disabled.
                  */
-               return -1u;
+               return (u16)-1u;
         default:
                 break;
         }

-       return exit_reason;
+       return exit_reason.full;
  }

  static noinstr void tdx_vcpu_enter_exit(struct kvm_vcpu *vcpu)

> [ ... ]
> 
>> @@ -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?

It won't return uninitialized or stale exit_reason. But changing the 
negative error codes to 0 is a wrong behavior. I will fix it.

The VMX handler looks to have the issue. I'll cook a separate patch to 
fix it.

>> +	}
>> +	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?

Same for the patch 1. This is not possible, TDX module itself should 
always support Bus Lock VM exit unless the hardware doesn't support it. 
But in the case of hardware doesn't it, KVM doesn't set 
kvm_caps.has_bus_lock_exit thus userspace cannot enable 
KVM_CAP_X86_BUS_LOCK_EXIT.

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


  reply	other threads:[~2026-08-05  7:53 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-05  3:12 [PATCH 0/2] KVM: TDX: Enable VM-DoS Prevention Features for TDX Xiaoyao Li
2026-08-05  3:12 ` [PATCH 1/2] KVM: TDX: Enable Notify VM exit Xiaoyao Li
2026-08-05  3:38   ` sashiko-bot
2026-08-05  4:16     ` Xiaoyao Li
2026-08-06 13:33   ` Nikolay Borisov
2026-08-06 13:50     ` Sean Christopherson
2026-08-07  0:27       ` Edgecombe, Rick P
2026-08-07  0:32         ` Sean Christopherson
2026-08-07  1:07           ` Xiaoyao Li
2026-08-07  6:46           ` Nikolay Borisov
2026-08-07  1:06     ` Xiaoyao Li
2026-08-05  3:12 ` [PATCH 2/2] KVM: TDX: Enable Bus Lock " Xiaoyao Li
2026-08-05  3:46   ` sashiko-bot
2026-08-05  7:53     ` Xiaoyao Li [this message]
2026-08-05 14:56       ` Sean Christopherson
2026-08-06  6:10         ` Xiaoyao Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4fedcaa6-d710-48f8-84c4-de682350bb2f@intel.com \
    --to=xiaoyao.li@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).