linux-kernel - Re: [RFC PATCH 06/12] KVM: TDX: Return -EIO, not -EINVAL, on a KVM_BUG

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLDDYo-b5ES-KBWW@google.com>
Date: Thu, 28 Aug 2025 14:00:18 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Rick P Edgecombe <rick.p.edgecombe@...el.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>, 
	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 Thu, 2025-08-28 at 12:21 -0700, Sean Christopherson wrote:
> > 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)
> 
> In general sounds good. But there it's a bit strange to specify them rcx, rdx,
> etc in a general helper. This is fallout from the existing chain of strange
> naming:
> 
> For example tdh_mem_range_block() plucks them from those registers and calls
> them ext_err1 due to their conditional meaning. Then KVM gives them some more
> meaning with 'entry' and 'level_state". Then prints them out as original
> register names. How about keeping the KVM names, like:
> 
> #define TDX_BUG_ON_2(__err, __fn, arg1, arg2, __kvm)		\
> 	__TDX_BUG_ON(__err, #__fn, __kvm, ", " #arg1 " 0x%llx, " #arg2 "
> 0x%llx", arg1, arg2)
> 
> so you get: entry: 0x00 level:0xF00

Ooh, nice, I'll tack on a patch.

> I *think* there is a way to make this work like var args and have a single
> function, but it becomes impossible for people to read.

Heh, and would probably take two months to decipher the compiler errors in order
to get it working :-)

> > 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);				\
> 
> There is a functional change 

Ugh, I missed that.  I'll do a prep change to make that explicit.

> in that the lock assert is not required if BUSY
> avoidance can be guaranteed to not happen. I don't think it should be needed
> today. I guess it's probably better to not rely on hitting rare races to catch
> an issue like that.

But that's not actually what the code does.  The lockdep assert won't trip because
KVM never removes S-EPT entries under read-lock:

		if (is_mirror_sp(sp)) {
			KVM_BUG_ON(shared, kvm);
			remove_external_spte(kvm, gfn, old_spte, level);
		}

Not because KVM actually guarantees -EBUSY is avoided.  So the current code is
flawed, it just doesn't cause problems.