[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?

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

[Carlos López]

On 5/18/26 5:16 PM, Sean Christopherson wrote:
> +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.

Right, so the existing UCNA behavior is correct, since it always logs
the error and delivery is gated on MCi_CTL2.

> 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?

The AMD manual does not clarify much either (9.3.1.3 Machine-Check
Global-Control Register):

  MCG_CTL is used by software to enable or disable the logging and
  reporting of machine-check errors from the implemented error-reporting
  banks. Depending on the implementation, detected errors from some
  error sources associated with a reporting bank that is disabled are
  still logged.

But my reading here is that MCG_CTL *does* control logging in the
general case, with some implementation-specific exceptions.

So if I get the picture correctly:

* UCNA error logging in KVM is always happens (good), and delivery is
  done via CMCI based on MCi_CTL2 (good). Nothing to fix here, unless
  MCG_CTL does affect logging (although it would be a bit strange for
  this to be true while CR4.MCE and MCi_CTL are explicitly mentioned to
  *not* have this behavior for CMCI).
* Non-UCNA error logging is broken, at least on the count that logging
  is gated based on MCi_CTL, when it should not. The spec is ambiguous
  on whether MCG_CTL controls logging for these. Current behavior in KVM
  is to *do* the gating based on MCG_CTL; the spec suggests that this
  may be correct at least on AMD (but it is not symmetric with MCi_CTL
  behavior).

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

[Carlos López]

On 6/2/26 9:56 PM, Carlos López wrote:
> ...
> * Non-UCNA error logging is broken, at least on the count that logging
>   is gated based on MCi_CTL, when it should not. The spec is ambiguous
>   on whether MCG_CTL controls logging for these. Current behavior in KVM
>   is to *do* the gating based on MCG_CTL; the spec suggests that this
>   may be correct at least on AMD (but it is not symmetric with MCi_CTL
>   behavior).

So something like this?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c9e40f2a5996..259dba2a3299 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5505,24 +5505,31 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
 	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 (mce->status & MCI_STATUS_UC) {
+		/*
+		 * Per the SDM, error logging always happens independently of
+		 * IA32_MCi_CTL
+		 */
+		if (banks[1] & MCI_STATUS_VAL)
+			mce->status |= MCI_STATUS_OVER;
+		banks[2] = mce->addr;
+		banks[3] = mce->misc;
+		banks[1] = mce->status;
+
+		/*
+		 * if IA32_MCi_CTL is not all 1s, the uncorrected error
+		 * reporting is disabled for the bank
+		 */
+		if (banks[0] != ~(u64)0)
+			return 0;
+
 		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)) {