On 4/11/22 21:08, Sean Christopherson wrote:
+ if (!(mcg_cap & MCG_CMCI_P) &&
+ (data || !msr_info->host_initiated))
This looks wrong, userspace should either be able to write the MSR or not, '0'
isn't special. Unless there's a danger to KVM, which I don't think there is,
userspace should be allowed to ignore architectural restrictions, i.e. bypass
the MCG_CMCI_P check, so that KVM doesn't create an unnecessary dependency between
ioctls. I.e. this should be:
if (!(mcg_cap & MCG_CMCI_P) && !msr_info->host_initiated)
return 1;
This is somewhat dangerous as it complicates (or removes) the invariants
that other code can rely on. Thus, usually, only the default value is
allowed for KVM_SET_MSR.
See commit b1e34d325397 ("KVM: x86: Forbid VMM to set SYNIC/STIMER MSRs
when SynIC wasn't activated", 2022-03-29) for a case where this practice
avoids a NULL pointer dereference. Though in there, Vitaly wrote:
Note, it would've been better to forbid writing anything to
SYNIC/STIMER MSRs, including zeroes, however, at least QEMU tries
clearing HV_X64_MSR_STIMER0_CONFIG without SynIC.
and I don't really agree with him, in that writing the default value of
an MSR is safe and should always be allowed for userspace.
Paolo
