On 05.02.24 19:42, Sean Christopherson wrote:
> On Sat, Feb 03, 2024, Mathias Krause wrote:
>> Commit 6abe9c1386e5 ("KVM: X86: Move ignore_msrs handling upper the
>> stack") changed the 'ignore_msrs' handling, including sanitizing return
>> values to the caller. This was fine until commit 12bc2132b15e ("KVM:
>> X86: Do the same ignore_msrs check for feature msrs") which allowed
>> non-existing feature MSRs to be ignored, i.e. to not generate an error
>> on the ioctl() level. It even tried to preserve the sanitization of the
>> return value. However, the logic is flawed, as '*data' will be
>> overwritten again with the uninitialized stack value of msr.data.
>
> Ugh, what a terrible commit. This makes no sense:
>
> Logically the ignore_msrs and report_ignored_msrs should also apply to feature
> MSRs. Add them in.
>
> The whole point of ignore_msrs was so that KVM could run _guest_ code that isn't
> aware it's running in a VM, and so attempts to access MSRs that the _guest_ thinks
> are always available.
Yeah, I was wondering that myself too. But I thought, maybe there's
buggy QEMU versions out there and it's because of that?
>
> The feature MSRs API is used only by userspace which obviously should know that
> it's dealing with KVM. Ignoring bad access from the host is just asinine.
>From a quick google search I found, enabling kvm.ignore_msrs is a common
suggestion to work around Windows bluescreens. I'm not a Windows user,
less so in VMs, so dunno if that's just snake oil or sometimes works by
chance because of returning "random" MSR values.
>
> At this point, it's not worth trying to revert that commit, but oof.
>
>> Fix this by simplifying the logic and always initializing msr.data,
>> vanishing the need for an additional error exit path.
>
> Out of curiosity, was this found by inspection, or by some other means? I'm quite
> surprised none of the sanitizers stumbled across this.
Manual inspection, yes. I was looking how MSRs are handled in general to
answer a different question for myself (related to FSGSBASE handling
resp. the lack thereof, but completely unrelated to this change) and
found this code just a little bit too ugly and looked a little closer.
>
>> Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")
>
> I'll apply this for 6.8. I think I'll also throw together a follow-up series to
> clean up some of this mess. There's no good reason this code has to be so grossly
> fragile.
Thanks,
Mathias
