| Message ID | 20220711232750.1092012-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Fix fault-related bugs in LTR/LLDT emulation | expand |
On Mon, 2022-07-11 at 23:27 +0000, Sean Christopherson wrote: > Wait to mark the TSS as busy during LTR emulation until after all fault > checks for the LTR have passed. Specifically, don't mark the TSS busy if > the new TSS base is non-canonical. Took me a while to notice it but I see the canonical check now, so the patch makes sense, and so: Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Unrelated, but I do wonder why we use cmpxchg_emulated for setting the busy bit, while we use write_segment_descriptor to set the accessed bit. Best regards, Maxim Levitsky > > Opportunistically drop the one-off !seg_desc.PRESENT check for TR as the > only reason for the early check was to avoid marking a !PRESENT TSS as > busy, i.e. the common !PRESENT is now done before setting the busy bit. > > Fixes: e37a75a13cda ("KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR") > Reported-by: syzbot+760a73552f47a8cd0fd9@syzkaller.appspotmail.com > Cc: stable@vger.kernel.org > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Cc: Hou Wenlong <houwenlong.hwl@antgroup.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/emulate.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 39ea9138224c..09e4b67b881f 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -1699,16 +1699,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt, > case VCPU_SREG_TR: > if (seg_desc.s || (seg_desc.type != 1 && seg_desc.type != 9)) > goto exception; > - if (!seg_desc.p) { > - err_vec = NP_VECTOR; > - goto exception; > - } > - old_desc = seg_desc; > - seg_desc.type |= 2; /* busy */ > - ret = ctxt->ops->cmpxchg_emulated(ctxt, desc_addr, &old_desc, &seg_desc, > - sizeof(seg_desc), &ctxt->exception); > - if (ret != X86EMUL_CONTINUE) > - return ret; > break; > case VCPU_SREG_LDTR: > if (seg_desc.s || seg_desc.type != 2) > @@ -1749,6 +1739,15 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt, > ((u64)base3 << 32), ctxt)) > return emulate_gp(ctxt, 0); > } > + > + if (seg == VCPU_SREG_TR) { > + old_desc = seg_desc; > + seg_desc.type |= 2; /* busy */ > + ret = ctxt->ops->cmpxchg_emulated(ctxt, desc_addr, &old_desc, &seg_desc, > + sizeof(seg_desc), &ctxt->exception); > + if (ret != X86EMUL_CONTINUE) > + return ret; > + } > load: > ctxt->ops->set_segment(ctxt, selector, &seg_desc, base3, seg); > if (desc)
On Tue, Jul 12, 2022, Maxim Levitsky wrote: > On Mon, 2022-07-11 at 23:27 +0000, Sean Christopherson wrote: > > Wait to mark the TSS as busy during LTR emulation until after all fault > > checks for the LTR have passed. Specifically, don't mark the TSS busy if > > the new TSS base is non-canonical. > > > Took me a while to notice it but I see the canonical check now, so the patch > makes sense, and so: > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > Unrelated, but I do wonder why we use cmpxchg_emulated for setting the busy > bit, while we use write_segment_descriptor to set the accessed bit. 99% certain it's a historical KVM bug in how it updates the accessed bit.
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 39ea9138224c..09e4b67b881f 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -1699,16 +1699,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt, case VCPU_SREG_TR: if (seg_desc.s || (seg_desc.type != 1 && seg_desc.type != 9)) goto exception; - if (!seg_desc.p) { - err_vec = NP_VECTOR; - goto exception; - } - old_desc = seg_desc; - seg_desc.type |= 2; /* busy */ - ret = ctxt->ops->cmpxchg_emulated(ctxt, desc_addr, &old_desc, &seg_desc, - sizeof(seg_desc), &ctxt->exception); - if (ret != X86EMUL_CONTINUE) - return ret; break; case VCPU_SREG_LDTR: if (seg_desc.s || seg_desc.type != 2) @@ -1749,6 +1739,15 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt, ((u64)base3 << 32), ctxt)) return emulate_gp(ctxt, 0); } + + if (seg == VCPU_SREG_TR) { + old_desc = seg_desc; + seg_desc.type |= 2; /* busy */ + ret = ctxt->ops->cmpxchg_emulated(ctxt, desc_addr, &old_desc, &seg_desc, + sizeof(seg_desc), &ctxt->exception); + if (ret != X86EMUL_CONTINUE) + return ret; + } load: ctxt->ops->set_segment(ctxt, selector, &seg_desc, base3, seg); if (desc)
Wait to mark the TSS as busy during LTR emulation until after all fault checks for the LTR have passed. Specifically, don't mark the TSS busy if the new TSS base is non-canonical. Opportunistically drop the one-off !seg_desc.PRESENT check for TR as the only reason for the early check was to avoid marking a !PRESENT TSS as busy, i.e. the common !PRESENT is now done before setting the busy bit. Fixes: e37a75a13cda ("KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR") Reported-by: syzbot+760a73552f47a8cd0fd9@syzkaller.appspotmail.com Cc: stable@vger.kernel.org Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Hou Wenlong <houwenlong.hwl@antgroup.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/emulate.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)