| Message ID | 20240203124522.592778-2-minipli@grsecurity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86 - misc fixes | expand |
On 2/3/2024 8:45 PM, 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. > > Fix this by simplifying the logic and always initializing msr.data, > vanishing the need for an additional error exit path. > > Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs") > Signed-off-by: Mathias Krause <minipli@grsecurity.net> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/kvm/x86.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 363b1c080205..13ec948f3241 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1704,22 +1704,17 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data) > struct kvm_msr_entry msr; > int r; > > + /* Unconditionally clear the output for simplicity */ > + msr.data = 0; > msr.index = index; > r = kvm_get_msr_feature(&msr); > > - if (r == KVM_MSR_RET_INVALID) { > - /* Unconditionally clear the output for simplicity */ > - *data = 0; > - if (kvm_msr_ignored_check(index, 0, false)) > - r = 0; > - } > - > - if (r) > - return r; > + if (r == KVM_MSR_RET_INVALID && kvm_msr_ignored_check(index, 0, false)) > + r = 0; > > *data = msr.data; > > - return 0; > + return r; > } > > static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
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.
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
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 363b1c080205..13ec948f3241 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1704,22 +1704,17 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data) struct kvm_msr_entry msr; int r; + /* Unconditionally clear the output for simplicity */ + msr.data = 0; msr.index = index; r = kvm_get_msr_feature(&msr); - if (r == KVM_MSR_RET_INVALID) { - /* Unconditionally clear the output for simplicity */ - *data = 0; - if (kvm_msr_ignored_check(index, 0, false)) - r = 0; - } - - if (r) - return r; + if (r == KVM_MSR_RET_INVALID && kvm_msr_ignored_check(index, 0, false)) + r = 0; *data = msr.data; - return 0; + return r; } static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
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. Fix this by simplifying the logic and always initializing msr.data, vanishing the need for an additional error exit path. Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs") Signed-off-by: Mathias Krause <minipli@grsecurity.net> --- arch/x86/kvm/x86.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)