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@xxxxxxx>
>>> ---
>>>  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).