linux-kernel - Re: [PATCH 01/28] KVM: SVM: Don't BUG if setting up the MSR intercept bitmaps fails

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: <aD8UolpjmE2zYOEB@google.com>
Date: Tue, 3 Jun 2025 08:28:34 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Borislav Petkov <bp@...en8.de>, Xin Li <xin@...or.com>, Dapeng Mi <dapeng1.mi@...ux.intel.com>
Subject: Re: [PATCH 01/28] KVM: SVM: Don't BUG if setting up the MSR intercept
 bitmaps fails

On Tue, Jun 03, 2025, Chao Gao wrote:
> On Thu, May 29, 2025 at 04:39:46PM -0700, Sean Christopherson wrote:
> >WARN and reject module loading if there is a problem with KVM's MSR
> >interception bitmaps.  Panicking the host in this situation is inexcusable
> >since it is trivially easy to propagate the error up the stack.
> >
> >Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> >---
> > arch/x86/kvm/svm/svm.c | 27 +++++++++++++++------------
> > 1 file changed, 15 insertions(+), 12 deletions(-)
> >
> >diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> >index 0ad1a6d4fb6d..bd75ff8e4f20 100644
> >--- a/arch/x86/kvm/svm/svm.c
> >+++ b/arch/x86/kvm/svm/svm.c
> >@@ -945,7 +945,7 @@ static void svm_msr_filter_changed(struct kvm_vcpu *vcpu)
> > 	}
> > }
> > 
> >-static void add_msr_offset(u32 offset)
> >+static int add_msr_offset(u32 offset)
> > {
> > 	int i;
> > 
> >@@ -953,7 +953,7 @@ static void add_msr_offset(u32 offset)
> > 
> > 		/* Offset already in list? */
> > 		if (msrpm_offsets[i] == offset)
> >-			return;
> >+			return 0;
> > 
> > 		/* Slot used by another offset? */
> > 		if (msrpm_offsets[i] != MSR_INVALID)
> >@@ -962,17 +962,13 @@ static void add_msr_offset(u32 offset)
> > 		/* Add offset to list */
> > 		msrpm_offsets[i] = offset;
> > 
> >-		return;
> >+		return 0;
> > 	}
> > 
> >-	/*
> >-	 * If this BUG triggers the msrpm_offsets table has an overflow. Just
> >-	 * increase MSRPM_OFFSETS in this case.
> >-	 */
> >-	BUG();
> >+	return -EIO;
> 
> Would -ENOSPC be more appropriate here?

Hmm, yeah.  IIRC, I initially had -ENOSPC, but switched to -EIO to be consistent
with how KVM typically reports its internal issues during module load.  But as
you point out, the error code isn't propagated up the stack.

> And, instead of returning an integer, using a boolean might be better since
> the error code isn't propagated upwards.

I strongly prefer to return 0/-errno in any function that isn't a clear cut
predicate, e.g. "return -EIO" is very obviously an error path (and -ENOSPC is
even better), whereas understanding "return false" requires reading the rest of
the function (and IMO this code isn't all that intuitive).

> > }
> > 
> >-static void init_msrpm_offsets(void)
> >+static int init_msrpm_offsets(void)
> > {
> > 	int i;
> > 
> >@@ -982,10 +978,13 @@ static void init_msrpm_offsets(void)
> > 		u32 offset;
> > 
> > 		offset = svm_msrpm_offset(direct_access_msrs[i].index);
> >-		BUG_ON(offset == MSR_INVALID);
> >+		if (WARN_ON(offset == MSR_INVALID))
> >+			return -EIO;
> > 
> >-		add_msr_offset(offset);
> >+		if (WARN_ON_ONCE(add_msr_offset(offset)))
> >+			return -EIO;
> > 	}
> >+	return 0;
> > }
> > 
> > void svm_copy_lbrs(struct vmcb *to_vmcb, struct vmcb *from_vmcb)
> >@@ -5511,7 +5510,11 @@ static __init int svm_hardware_setup(void)
> > 	memset(iopm_va, 0xff, PAGE_SIZE * (1 << order));
> > 	iopm_base = __sme_page_pa(iopm_pages);
> > 
> >-	init_msrpm_offsets();
> >+	r = init_msrpm_offsets();
> >+	if (r) {
> >+		__free_pages(__sme_pa_to_page(iopm_base), get_order(IOPM_SIZE));
> 
> __free_pages(iopm_pages, order);

Oh, yeah, good call.

> And we can move init_msrpm_offsets() above the allocation of iopm_pages to
> avoid the need for rewinding. But I don't have a strong opinion on this, as
> it goes beyond a simple change to the return type.

I considered that too.  I decided not to bother since this code goes away by the
end of the series.  I thought about skipping this patch entirely, but I kept it
since it should be easy to backport, e.g. if someone wants make their tree more
developer friendly, and on the off chance the patches later in the series need to
be droppped or reverted.