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

[Sean Christopherson <seanjc@google.com>]

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.