[PATCH] KVM: x86: Fix array_index_nospec() protection in kvm_vcpu_ioctl_x86_set_mce()

[PATCH] KVM: x86: Fix array_index_nospec() protection in kvm_vcpu_ioctl_x86_set_mce()

[Carlos López]

Commit aebc3ca19063 ("KVM: x86: Enable CMCI capability by default and
handle injected UCNA errors") introduced kvm_vcpu_x86_set_ucna(), which
accesses @vcpu->arch.mci_ctl2_banks[] using @mce->bank as the index. The
@mce struct is user-controlled, provided via the KVM_X86_SET_MCE ioctl.

The caller of this function, kvm_vcpu_ioctl_x86_set_mce(), bounds-checks
@mce->bank and applies array_index_nospec() to advance the @banks
pointer, but @mce->bank itself is passed through unclamped. On a
speculative path that bypasses the bounds check, the raw @mce->bank
value can index mci_ctl2_banks[] out-of-bounds.

In practice this is a very weak gadget, and would at most allow leaking
a single bit in a 64-bit integer, but prevent potential future issues by
clamping @mce->bank in place with array_index_nospec(), before passing
the struct to kvm_vcpu_x86_set_ucna().

Fixes: aebc3ca19063 ("KVM: x86: Enable CMCI capability by default and handle injected UCNA errors")
Signed-off-by: Carlos López <clopez@suse.de>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 209eae67ab18..2d2415031267 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5497,7 +5497,8 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
 	if (mce->bank >= bank_num || !(mce->status & MCI_STATUS_VAL))
 		return -EINVAL;
 
-	banks += array_index_nospec(4 * mce->bank, 4 * bank_num);
+	mce->bank = array_index_nospec(mce->bank, bank_num);
+	banks += 4 * mce->bank;
 
 	if (is_ucna(mce))
 		return kvm_vcpu_x86_set_ucna(vcpu, mce, banks);

base-commit: a9512a611bd030088f13477258d1f8103cceaa40
-- 
2.51.0

Re: [PATCH] KVM: x86: Fix array_index_nospec() protection in kvm_vcpu_ioctl_x86_set_mce()

[Sean Christopherson]

On Sat, May 16, 2026, Carlos López wrote:
> Commit aebc3ca19063 ("KVM: x86: Enable CMCI capability by default and
> handle injected UCNA errors") introduced kvm_vcpu_x86_set_ucna(), which
> accesses @vcpu->arch.mci_ctl2_banks[] using @mce->bank as the index. The
> @mce struct is user-controlled, provided via the KVM_X86_SET_MCE ioctl.
> 
> The caller of this function, kvm_vcpu_ioctl_x86_set_mce(), bounds-checks
> @mce->bank and applies array_index_nospec() to advance the @banks
> pointer, but @mce->bank itself is passed through unclamped. On a
> speculative path that bypasses the bounds check, the raw @mce->bank
> value can index mci_ctl2_banks[] out-of-bounds.
> 
> In practice this is a very weak gadget, and would at most allow leaking
> a single bit in a 64-bit integer, but prevent potential future issues by
> clamping @mce->bank in place with array_index_nospec(), before passing
> the struct to kvm_vcpu_x86_set_ucna().
> 
> Fixes: aebc3ca19063 ("KVM: x86: Enable CMCI capability by default and handle injected UCNA errors")
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
>  arch/x86/kvm/x86.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 209eae67ab18..2d2415031267 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5497,7 +5497,8 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
>  	if (mce->bank >= bank_num || !(mce->status & MCI_STATUS_VAL))
>  		return -EINVAL;
>  
> -	banks += array_index_nospec(4 * mce->bank, 4 * bank_num);
> +	mce->bank = array_index_nospec(mce->bank, bank_num);
> +	banks += 4 * mce->bank;
>  
>  	if (is_ucna(mce))
>  		return kvm_vcpu_x86_set_ucna(vcpu, mce, banks);

As a follow-up, I think we should fold kvm_vcpu_x86_set_ucna() into
kvm_vcpu_ioctl_x86_set_mce().  Splitting it to a separately helper makes it
unnecessarily difficult to see the usage of mce->bank.

Hmm, and isn't the handling of UNCA errors misplaced?  Per the SDM, the only
allowed values for IA32_MCG_CTL are -1ull and 0.

  IA32_MCG_CTL controls the reporting of machine-check exceptions. If present,
  writing 1s to this register enables machine-check features and writing all 0s
  disables machine-check features. All other values are undefined and/or
  implementation specific.

I don't see anything the SDM that exempts UNCA from that logic.  Ditto for the
similar IA32_MCi_CTL check.  So on top of your fix, over two patches, I think we
should end up with this?

	mce->bank = array_index_nospec(mce->bank, bank_num);
	banks += 4 * mce->bank;

	/*
	 * if IA32_MCG_CTL is not all 1s, the uncorrected error
	 * reporting is disabled
	 */
	if ((mce->status & MCI_STATUS_UC) && (mcg_cap & MCG_CTL_P) &&
	    vcpu->arch.mcg_ctl != ~(u64)0)
		return 0;
	/*
	 * if IA32_MCi_CTL is not all 1s, the uncorrected error
	 * reporting is disabled for the bank
	 */
	if ((mce->status & MCI_STATUS_UC) && banks[0] != ~(u64)0)
		return 0;

	if (is_ucna(mce)) {
		banks[1] = mce->status;
		banks[2] = mce->addr;
		banks[3] = mce->misc;
		vcpu->arch.mcg_status = mce->mcg_status;

		if (lapic_in_kernel(vcpu) && (mcg_cap & MCG_CMCI_P) &&
		    (vcpu->arch.mci_ctl2_banks[mce->bank] & MCI_CTL2_CMCI_EN))
			kvm_apic_local_deliver(vcpu->arch.apic, APIC_LVTCMCI);
	} else if (mce->status & MCI_STATUS_UC) {
		if ((vcpu->arch.mcg_status & MCG_STATUS_MCIP) ||
		    !kvm_is_cr4_bit_set(vcpu, X86_CR4_MCE)) {
			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
			return 0;
		}
		if (banks[1] & MCI_STATUS_VAL)
			mce->status |= MCI_STATUS_OVER;
		banks[2] = mce->addr;
		banks[3] = mce->misc;
		vcpu->arch.mcg_status = mce->mcg_status;
		banks[1] = mce->status;
		kvm_queue_exception(vcpu, MC_VECTOR);
	} else if (!(banks[1] & MCI_STATUS_VAL)
		   || !(banks[1] & MCI_STATUS_UC)) {
		if (banks[1] & MCI_STATUS_VAL)
			mce->status |= MCI_STATUS_OVER;
		banks[2] = mce->addr;
		banks[3] = mce->misc;
		banks[1] = mce->status;
	} else
		banks[1] |= MCI_STATUS_OVER;
	return 0;

Unless I'm wrong, I'll send a v2 with your fix plus two more patches.

Re: [PATCH] KVM: x86: Fix array_index_nospec() protection in kvm_vcpu_ioctl_x86_set_mce()

[Sean Christopherson]

+Tony (questions regarding IA32_MCG_CTL and IA32_MCi_CTL behavior)

On Mon, May 18, 2026, Sean Christopherson wrote:
> On Sat, May 16, 2026, Carlos López wrote:
> > Commit aebc3ca19063 ("KVM: x86: Enable CMCI capability by default and
> > handle injected UCNA errors") introduced kvm_vcpu_x86_set_ucna(), which
> > accesses @vcpu->arch.mci_ctl2_banks[] using @mce->bank as the index. The
> > @mce struct is user-controlled, provided via the KVM_X86_SET_MCE ioctl.
> > 
> > The caller of this function, kvm_vcpu_ioctl_x86_set_mce(), bounds-checks
> > @mce->bank and applies array_index_nospec() to advance the @banks
> > pointer, but @mce->bank itself is passed through unclamped. On a
> > speculative path that bypasses the bounds check, the raw @mce->bank
> > value can index mci_ctl2_banks[] out-of-bounds.
> > 
> > In practice this is a very weak gadget, and would at most allow leaking
> > a single bit in a 64-bit integer, but prevent potential future issues by
> > clamping @mce->bank in place with array_index_nospec(), before passing
> > the struct to kvm_vcpu_x86_set_ucna().
> > 
> > Fixes: aebc3ca19063 ("KVM: x86: Enable CMCI capability by default and handle injected UCNA errors")
> > Signed-off-by: Carlos López <clopez@suse.de>
> > ---
> >  arch/x86/kvm/x86.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 209eae67ab18..2d2415031267 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5497,7 +5497,8 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
> >  	if (mce->bank >= bank_num || !(mce->status & MCI_STATUS_VAL))
> >  		return -EINVAL;
> >  
> > -	banks += array_index_nospec(4 * mce->bank, 4 * bank_num);
> > +	mce->bank = array_index_nospec(mce->bank, bank_num);
> > +	banks += 4 * mce->bank;
> >  
> >  	if (is_ucna(mce))
> >  		return kvm_vcpu_x86_set_ucna(vcpu, mce, banks);
> 
> As a follow-up, I think we should fold kvm_vcpu_x86_set_ucna() into
> kvm_vcpu_ioctl_x86_set_mce().  Splitting it to a separately helper makes it
> unnecessarily difficult to see the usage of mce->bank.
> 
> Hmm, and isn't the handling of UNCA errors misplaced?  Per the SDM, the only
> allowed values for IA32_MCG_CTL are -1ull and 0.
> 
>   IA32_MCG_CTL controls the reporting of machine-check exceptions. If present,
>   writing 1s to this register enables machine-check features and writing all 0s
>   disables machine-check features. All other values are undefined and/or
>   implementation specific.
> 
> I don't see anything the SDM that exempts UNCA from that logic.  Ditto for the
> similar IA32_MCi_CTL check.  So on top of your fix, over two patches, I think we
> should end up with this?

...

> Unless I'm wrong, I'll send a v2 with your fix plus two more patches.

Heh, I was partially wrong.  I think.  The IA32_MCi_CTL MSRs control whether or
not a #MC is reported, but don't change error _logging_.

  Setting an EEj flag enables signaling #MC of the associated error and clearing
  it disables signaling of the error. Error logging happens regardless of the
  setting of these bits. The processor drops writes to bits that are not implemented.

Ah, I was definitely wrong.  Section 17.5 CORRECTED MACHINE CHECK ERROR INTERRUPT
explicitly says so:

  CMCI is not affected by the CR4.MCE bit, and it is not affected by the
  IA32_MCi_CTL MSRs.

Actually, that means the existing code is wrong, and has been since commit
890ca9aefa78 ("KVM: Add MCE support").

Hrm, and I'm not convinced KVM's handling of IA32_MCG_CTL is correct either.  The
SDM says it controls reporting of _exceptions_, it doesn't say anything about
disabling loggin of errors.

  IA32_MCG_CTL controls the reporting of machine-check exceptions.


Tony,

Does clearing IA32_MCG_CTL affect error logging *and* #MC generation, or just #MC
generation?  And if it impacts logging, does it also apply to UCNA errors?