[PATCH 0/3] KVM: x86: Fix fault-related bugs in LTR/LLDT emulation

[PATCH 0/3] KVM: x86: Fix fault-related bugs in LTR/LLDT emulation

[Sean Christopherson]

Patch 1 fixes a bug found by syzkaller where KVM attempts to set the
TSS.busy bit during LTR before checking that the new TSS.base is valid.

Patch 2 fixes a bug found by inspection (when reading the APM to verify
the non-canonical logic is correct) where KVM doesn't provide the correct
error code if the new TSS.base is non-canonical.

Patch 3 makes the "dangling userspace I/O" WARN_ON two separate WARN_ON_ONCE
so that a KVM bug doesn't spam the kernel log (keeping the WARN is desirable
specifically to detect these types of bugs).

Sean Christopherson (3):
  KVM: x86: Mark TSS busy during LTR emulation _after_ all fault checks
  KVM: x86: Set error code to segment selector on LLDT/LTR non-canonical
    #GP
  KVM: x86: WARN only once if KVM leaves a dangling userspace I/O
    request

 arch/x86/kvm/emulate.c | 23 +++++++++++------------
 arch/x86/kvm/x86.c     |  6 ++++--
 2 files changed, 15 insertions(+), 14 deletions(-)


base-commit: b9b71f43683ae9d76b0989249607bbe8c9eb6c5c
-- 
2.37.0.144.g8ac04bfd2-goog

[PATCH 1/3] KVM: x86: Mark TSS busy during LTR emulation _after_ all fault checks

[Sean Christopherson]

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(-)

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)
-- 
2.37.0.144.g8ac04bfd2-goog

Re: [PATCH 1/3] KVM: x86: Mark TSS busy during LTR emulation _after_ all fault checks

[Maxim Levitsky]

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)

Re: [PATCH 1/3] KVM: x86: Mark TSS busy during LTR emulation _after_ all fault checks

[Sean Christopherson]

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.

[PATCH 2/3] KVM: x86: Set error code to segment selector on LLDT/LTR non-canonical #GP

[Sean Christopherson]

When injecting a #GP on LLDT/LTR due to a non-canonical LDT/TSS base, set
the error code to the selector.  Intel SDM's says nothing about the #GP,
but AMD's APM explicitly states that both LLDT and LTR set the error code
to the selector, not zero.

Note, a non-canonical memory operand on LLDT/LTR does generate a #GP(0),
but the KVM code in question is specific to the base from the descriptor.

Fixes: e37a75a13cda ("KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 09e4b67b881f..bd9e9c5627d0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1736,8 +1736,8 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		if (ret != X86EMUL_CONTINUE)
 			return ret;
 		if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
-				((u64)base3 << 32), ctxt))
-			return emulate_gp(ctxt, 0);
+						 ((u64)base3 << 32), ctxt))
+			return emulate_gp(ctxt, err_code);
 	}
 
 	if (seg == VCPU_SREG_TR) {
-- 
2.37.0.144.g8ac04bfd2-goog

Re: [PATCH 2/3] KVM: x86: Set error code to segment selector on LLDT/LTR non-canonical #GP

[Maxim Levitsky]

On Mon, 2022-07-11 at 23:27 +0000, Sean Christopherson wrote:
> When injecting a #GP on LLDT/LTR due to a non-canonical LDT/TSS base, set
> the error code to the selector.  Intel SDM's says nothing about the #GP,
> but AMD's APM explicitly states that both LLDT and LTR set the error code
> to the selector, not zero.
> 
> Note, a non-canonical memory operand on LLDT/LTR does generate a #GP(0),
> but the KVM code in question is specific to the base from the descriptor.
> 
> Fixes: e37a75a13cda ("KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/emulate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 09e4b67b881f..bd9e9c5627d0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1736,8 +1736,8 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>                 if (ret != X86EMUL_CONTINUE)
>                         return ret;
>                 if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
> -                               ((u64)base3 << 32), ctxt))
> -                       return emulate_gp(ctxt, 0);
> +                                                ((u64)base3 << 32), ctxt))
> +                       return emulate_gp(ctxt, err_code);
>         }
>  
>         if (seg == VCPU_SREG_TR) {

I guess this is the quote from AMD's manual (might we worth to add to the source?)


"The 64-bit system-segment base address must be in canonical form. Otherwise, a general-protection
exception occurs with a selector error-code, #GP(selector), when the system segment is loaded.
System-segment limit values are checked by the processor in both 64-bit and compatibility modes,
under the control of the granularity (G) bit."

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 2/3] KVM: x86: Set error code to segment selector on LLDT/LTR non-canonical #GP

[Sean Christopherson]

On Tue, Jul 12, 2022, Maxim Levitsky wrote:
> On Mon, 2022-07-11 at 23:27 +0000, Sean Christopherson wrote:
> > When injecting a #GP on LLDT/LTR due to a non-canonical LDT/TSS base, set
> > the error code to the selector.  Intel SDM's says nothing about the #GP,
> > but AMD's APM explicitly states that both LLDT and LTR set the error code
> > to the selector, not zero.
> > 
> > Note, a non-canonical memory operand on LLDT/LTR does generate a #GP(0),
> > but the KVM code in question is specific to the base from the descriptor.
> > 
> > Fixes: e37a75a13cda ("KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/emulate.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 09e4b67b881f..bd9e9c5627d0 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -1736,8 +1736,8 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
> >                 if (ret != X86EMUL_CONTINUE)
> >                         return ret;
> >                 if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
> > -                               ((u64)base3 << 32), ctxt))
> > -                       return emulate_gp(ctxt, 0);
> > +                                                ((u64)base3 << 32), ctxt))
> > +                       return emulate_gp(ctxt, err_code);
> >         }
> >  
> >         if (seg == VCPU_SREG_TR) {
> 
> I guess this is the quote from AMD's manual (might we worth to add to the source?)

Eh, probably not worth it.  Anyone working on segmentation emulation is already
up to their eyeballs in the SDM/APM.

[PATCH 3/3] KVM: x86: WARN only once if KVM leaves a dangling userspace I/O request

[Sean Christopherson]

Change a WARN_ON() to separate WARN_ON_ONCE() if KVM has an outstanding
PIO or MMIO request without an associated callback, i.e. if KVM queued a
userspace I/O exit but didn't actually exit to userspace before moving
on to something else.  Warning on every KVM_RUN risks spamming the kernel
if KVM gets into a bad state.  Opportunistically split the WARNs so that
it's easier to triage failures when a WARN fires.

Deliberately do not use KVM_BUG_ON(), i.e. don't kill the VM.  While the
WARN is all but guaranteed to fire if and only if there's a KVM bug, a
dangling I/O request does not present a danger to KVM (that flag is truly
truly consumed only in a single emulator path), and any such bug is
unlikely to be fatal to the VM (KVM essentially failed to do something it
shouldn't have tried to do in the first place).  In other words, note the
bug, but let the VM keep running.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 567d13405445..50dc55996416 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10847,8 +10847,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		r = cui(vcpu);
 		if (r <= 0)
 			goto out;
-	} else
-		WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed);
+	} else {
+		WARN_ON_ONCE(vcpu->arch.pio.count);
+		WARN_ON_ONCE(vcpu->mmio_needed);
+	}
 
 	if (kvm_run->immediate_exit) {
 		r = -EINTR;
-- 
2.37.0.144.g8ac04bfd2-goog

Re: [PATCH 3/3] KVM: x86: WARN only once if KVM leaves a dangling userspace I/O request

[Maxim Levitsky]

On Mon, 2022-07-11 at 23:27 +0000, Sean Christopherson wrote:
> Change a WARN_ON() to separate WARN_ON_ONCE() if KVM has an outstanding
> PIO or MMIO request without an associated callback, i.e. if KVM queued a
> userspace I/O exit but didn't actually exit to userspace before moving
> on to something else.  Warning on every KVM_RUN risks spamming the kernel
> if KVM gets into a bad state.  Opportunistically split the WARNs so that
> it's easier to triage failures when a WARN fires.
> 
> Deliberately do not use KVM_BUG_ON(), i.e. don't kill the VM.  While the
> WARN is all but guaranteed to fire if and only if there's a KVM bug, a
> dangling I/O request does not present a danger to KVM (that flag is truly
> truly consumed only in a single emulator path), and any such bug is
> unlikely to be fatal to the VM (KVM essentially failed to do something it
> shouldn't have tried to do in the first place).  In other words, note the
> bug, but let the VM keep running.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 567d13405445..50dc55996416 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10847,8 +10847,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>                 r = cui(vcpu);
>                 if (r <= 0)
>                         goto out;
> -       } else
> -               WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed);
> +       } else {
> +               WARN_ON_ONCE(vcpu->arch.pio.count);
> +               WARN_ON_ONCE(vcpu->mmio_needed);
> +       }
>  
>         if (kvm_run->immediate_exit) {
>                 r = -EINTR;

At some point in the future, the checkpatch.pl should start to WARN the
patch submitter if WARN_ON and not WARN_ON_ONCE was used ;-)

It already bugs the user about BUG_ON ;-)

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 0/3] KVM: x86: Fix fault-related bugs in LTR/LLDT emulation

[Nadav Amit]

On Jul 11, 2022, at 4:27 PM, Sean Christopherson <seanjc@google.com> wrote:

> Patch 1 fixes a bug found by syzkaller where KVM attempts to set the
> TSS.busy bit during LTR before checking that the new TSS.base is valid.
> 
> Patch 2 fixes a bug found by inspection (when reading the APM to verify
> the non-canonical logic is correct) where KVM doesn't provide the correct
> error code if the new TSS.base is non-canonical.
> 
> Patch 3 makes the "dangling userspace I/O" WARN_ON two separate WARN_ON_ONCE
> so that a KVM bug doesn't spam the kernel log (keeping the WARN is desirable
> specifically to detect these types of bugs).

Hi Sean,

If/when you find that I screwed up, would you be kind enough to cc me?

Very likely I won’t be able to assist too much in fixing the bugs under my
current affiliation, but it is always interesting to see the escapees of
Intel’s validation tools… ;-)

Only if you can.

Thanks,
Nadav

[ p.s. - please use my gmail account for the matter ]

Re: [PATCH 0/3] KVM: x86: Fix fault-related bugs in LTR/LLDT emulation

[Sean Christopherson]

On Mon, Jul 11, 2022, Nadav Amit wrote:
> On Jul 11, 2022, at 4:27 PM, Sean Christopherson <seanjc@google.com> wrote:
> 
> > Patch 1 fixes a bug found by syzkaller where KVM attempts to set the
> > TSS.busy bit during LTR before checking that the new TSS.base is valid.
> > 
> > Patch 2 fixes a bug found by inspection (when reading the APM to verify
> > the non-canonical logic is correct) where KVM doesn't provide the correct
> > error code if the new TSS.base is non-canonical.
> > 
> > Patch 3 makes the "dangling userspace I/O" WARN_ON two separate WARN_ON_ONCE
> > so that a KVM bug doesn't spam the kernel log (keeping the WARN is desirable
> > specifically to detect these types of bugs).
> 
> Hi Sean,
> 
> If/when you find that I screwed up, would you be kind enough to cc me?

Will do!

> Very likely I won’t be able to assist too much in fixing the bugs under my
> current affiliation, but it is always interesting to see the escapees of
> Intel’s validation tools… ;-)
>
> Only if you can.
> 
> Thanks,
> Nadav
> 
> [ p.s. - please use my gmail account for the matter ]

Yep, still got an alias for ya :-)