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