| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aLCsM6DShlGDxPOd@google.com>
Date: Thu, 28 Aug 2025 12:21:23 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Rick P Edgecombe <rick.p.edgecombe@...el.com>
Cc: "pbonzini@...hat.com" <pbonzini@...hat.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
Vishal Annapurve <vannapurve@...gle.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Yan Y Zhao <yan.y.zhao@...el.com>,
"michael.roth@....com" <michael.roth@....com>, Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RFC PATCH 06/12] KVM: TDX: Return -EIO, not -EINVAL, on a
KVM_BUG_ON() condition
On Thu, Aug 28, 2025, Rick P Edgecombe wrote:
> On Tue, 2025-08-26 at 17:05 -0700, Sean Christopherson wrote:
> > Return -EIO when a KVM_BUG_ON() is tripped, as KVM's ABI is to return -EIO
> > when a VM has been killed due to a KVM bug, not -EINVAL.
> >
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > ---
> > arch/x86/kvm/vmx/tdx.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 9fb6e5f02cc9..ef4ffcad131f 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1624,7 +1624,7 @@ static int tdx_mem_page_record_premap_cnt(struct kvm *kvm, gfn_t gfn,
> > struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> >
> > if (KVM_BUG_ON(kvm->arch.pre_fault_allowed, kvm))
> > - return -EINVAL;
> > + return -EIO;
> >
> > /* nr_premapped will be decreased when tdh_mem_page_add() is called. */
> > atomic64_inc(&kvm_tdx->nr_premapped);
> > @@ -1638,7 +1638,7 @@ static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn,
> >
> > /* TODO: handle large pages. */
> > if (KVM_BUG_ON(level != PG_LEVEL_4K, kvm))
> > - return -EINVAL;
> > + return -EIO;
> >
> > /*
> > * Read 'pre_fault_allowed' before 'kvm_tdx->state'; see matching
> > @@ -1849,7 +1849,7 @@ static int tdx_sept_free_private_spt(struct kvm *kvm, gfn_t gfn,
> > * and slot move/deletion.
> > */
> > if (KVM_BUG_ON(is_hkid_assigned(kvm_tdx), kvm))
> > - return -EINVAL;
> > + return -EIO;
> >
> > /*
> > * The HKID assigned to this TD was already freed and cache was
> > @@ -1870,7 +1870,7 @@ static int tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> > * there can't be anything populated in the private EPT.
> > */
> > if (KVM_BUG_ON(!is_hkid_assigned(to_kvm_tdx(kvm)), kvm))
> > - return -EINVAL;
> > + return -EIO;
> >
> > ret = tdx_sept_zap_private_spte(kvm, gfn, level, page);
> > if (ret <= 0)
>
>
> Did you miss?
I did indeed.
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index f9ac590e8ff0..fd1b8fea55a9 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1656,10 +1656,10 @@ static int tdx_sept_drop_private_spte(struct kvm *kvm,
> gfn_t gfn,
>
> /* TODO: handle large pages. */
> if (KVM_BUG_ON(level != PG_LEVEL_4K, kvm))
> - return -EINVAL;
> + return -EIO;
>
> if (KVM_BUG_ON(!is_hkid_assigned(kvm_tdx), kvm))
> - return -EINVAL;
> + return -EIO;
>
> /*
> * When zapping private page, write lock is held. So no race condition
>
>
> We really have a lot of KVM_BUG_ON()s in tdx code. I hesitate to pull them out
> but it feels a bit gratuitous.
Generally speaking, the number of KVM_BUG_ON()s is fine. What we can do though
is reduce the amount of boilerplate and the number of paths the propagate a SEAMCALL
err through multiple layers, e.g. by eliminating single-use helpers (which is made
easier by reducing boilerplate and thus lines of code).
Concretely, if we combine the KVM_BUG_ON() usage with pr_tdx_error():
#define __TDX_BUG_ON(__err, __fn_str, __kvm, __fmt, __args...) \
({ \
struct kvm *_kvm = (__kvm); \
bool __ret = !!(__err); \
\
if (WARN_ON_ONCE(__ret && (!_kvm || !_kvm->vm_bugged))) { \
if (_kvm) \
kvm_vm_bugged(_kvm); \
pr_err_ratelimited("SEAMCALL " __fn_str " failed: 0x%llx" \
__fmt "\n", __err, __args); \
} \
unlikely(__ret); \
})
#define TDX_BUG_ON(__err, __fn, __kvm) \
__TDX_BUG_ON(__err, #__fn, __kvm, "%s", "")
#define TDX_BUG_ON_1(__err, __fn, __rcx, __kvm) \
__TDX_BUG_ON(__err, #__fn, __kvm, ", rcx 0x%llx", __rcx)
#define TDX_BUG_ON_2(__err, __fn, __rcx, __rdx, __kvm) \
__TDX_BUG_ON(__err, #__fn, __kvm, ", rcx 0x%llx, rdx 0x%llx", __rcx, __rdx)
#define TDX_BUG_ON_3(__err, __fn, __rcx, __rdx, __r8, __kvm) \
__TDX_BUG_ON(__err, #__fn, __kvm, ", rcx 0x%llx, rdx 0x%llx, r8 0x%llx", __rcx, __rdx, __r8)
And a macro to handle retry when kicking vCPUs out of the guest:
#define tdh_do_no_vcpus(tdh_func, kvm, args...) \
({ \
struct kvm_tdx *__kvm_tdx = to_kvm_tdx(kvm); \
u64 __err; \
\
lockdep_assert_held_write(&kvm->mmu_lock); \
\
__err = tdh_func(args); \
if (unlikely(tdx_operand_busy(__err))) { \
WRITE_ONCE(__kvm_tdx->wait_for_sept_zap, true); \
kvm_make_all_cpus_request(kvm, KVM_REQ_OUTSIDE_GUEST_MODE); \
\
__err = tdh_func(args); \
\
WRITE_ONCE(__kvm_tdx->wait_for_sept_zap, false); \
} \
__err; \
})
And do a bit of massaging, then we can end up e.g. this, which IMO is much easier
to follow than the current form of tdx_sept_remove_private_spte(), which has
several duplicate sanity checks and error handlers.
The tdh_do_no_vcpus() macro is a little mean, but I think it's a net positive
as eliminates quite a lot of "noise", and thus makes it easier to focus on the
logic. And alternative to a trampoline macro would be to implement a guard()
and then do a scoped_guard(), but I think that'd be just as hard to read, and
would require almost as much boilerplate as there is today.
static void tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
enum pg_level level, u64 spte)
{
struct page *page = pfn_to_page(spte_to_pfn(spte));
int tdx_level = pg_level_to_tdx_sept_level(level);
struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
gpa_t gpa = gfn_to_gpa(gfn);
u64 err, entry, level_state;
/*
* HKID is released after all private pages have been removed, and set
* before any might be populated. Warn if zapping is attempted when
* there can't be anything populated in the private EPT.
*/
if (KVM_BUG_ON(!is_hkid_assigned(to_kvm_tdx(kvm)), kvm))
return;
/* TODO: handle large pages. */
if (KVM_BUG_ON(level != PG_LEVEL_4K, kvm))
return;
err = tdh_do_no_vcpus(tdh_mem_range_block, kvm, &kvm_tdx->td, gpa,
tdx_level, &entry, &level_state);
if (TDX_BUG_ON_2(err, TDH_MEM_RANGE_BLOCK, entry, level_state, kvm))
return;
/*
* TDX requires TLB tracking before dropping private page. Do
* it here, although it is also done later.
*/
tdx_track(kvm);
/*
* When zapping private page, write lock is held. So no race condition
* with other vcpu sept operation.
* Race with TDH.VP.ENTER due to (0-step mitigation) and Guest TDCALLs.
*/
err = tdh_do_no_vcpus(tdh_mem_page_remove, kvm, &kvm_tdx->td, gpa,
tdx_level, &entry, &level_state);
if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_REMOVE, entry, level_state, kvm))
return;
err = tdh_phymem_page_wbinvd_hkid((u16)kvm_tdx->hkid, page);
if (TDX_BUG_ON(err, TDH_PHYMEM_PAGE_WBINVD, kvm))
return;
tdx_clear_page(page);
}
Powered by blists - more mailing lists