| Message ID | 20250416134326.1342-1-chenyufeng@iie.ac.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kvm: x86: Don't report guest userspace emulation error to userspace in kvm_task_switch() | expand |
On Wed, Apr 16, 2025, Chen Yufeng wrote: > This patch prevents that emulation failures which result from emulating > task switch for an L2-Guest results in being reported to userspace. > > Without this patch a malicious L2-Guest would be able to kill the L1 by > triggering a race-condition between an vmexit and the task switch emulator. Only if L1 doesn't intercept task switches, which is only possible on SVM (they are a mandatory intercept on VMX). If L1 is deferring task switch emulation to L0, then IMO L0 is well within its rights to exit to userspace if KVM can't emulate the task switch. So unless I'm missing something, I vote to keep the code as-is. > This patch is smiliar to commit fc3a9157d314 ("KVM: X86: Don't report L2 > emulation failures to user-space") Generic emulation is different. There are legitimate scenarios where KVM needs to emulate L2 instructions, without L1's explicit consent, and so KVM needs to guard against L2 playing games with its code stream. Task switches are very different. KVM doesn't fetch from the code stream, i.e. L2 can't play TLB games, and I highly doubt there is a real world hypervisor that doesn't intercept task switches. > Fixes: 1051778f6e1e ("KVM: x86: Handle emulation failure directly in kvm_task_switch()") > > Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn> > --- > arch/x86/kvm/x86.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3712dde0bf9d..b22be88196ed 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -11874,9 +11874,11 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, > */ > if (ret || vcpu->mmio_needed) { > vcpu->mmio_needed = false; > - vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > - vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > - vcpu->run->internal.ndata = 0; > + if (!is_guest_mode(vcpu)) { > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > + vcpu->run->internal.ndata = 0; > + } > return 0; > } > > -- > 2.34.1 >
> On Wed, Apr 16, 2025, Chen Yufeng wrote: > > This patch prevents that emulation failures which result from emulating > > task switch for an L2-Guest results in being reported to userspace. > > > > Without this patch a malicious L2-Guest would be able to kill the L1 by > > triggering a race-condition between an vmexit and the task switch emulator. > > Only if L1 doesn't intercept task switches, which is only possible on SVM (they > are a mandatory intercept on VMX). If L1 is deferring task switch emulation to > L0, then IMO L0 is well within its rights to exit to userspace if KVM can't > emulate the task switch. > > So unless I'm missing something, I vote to keep the code as-is. > > > This patch is smiliar to commit fc3a9157d314 ("KVM: X86: Don't report L2 > > emulation failures to user-space") > > Generic emulation is different. There are legitimate scenarios where KVM needs > to emulate L2 instructions, without L1's explicit consent, and so KVM needs to > guard against L2 playing games with its code stream. > > Task switches are very different. KVM doesn't fetch from the code stream, i.e. > L2 can't play TLB games, and I highly doubt there is a real world hypervisor > that doesn't intercept task switches. Thank you for clarifying! Your explanation about this function makes sense. I agree there's no vulnerability here, and the existing code is justified. Appreciate the thorough review! > > Fixes: 1051778f6e1e ("KVM: x86: Handle emulation failure directly in kvm_task_switch()") > > > > Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn> > > --- > > arch/x86/kvm/x86.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 3712dde0bf9d..b22be88196ed 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -11874,9 +11874,11 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, > > */ > > if (ret || vcpu->mmio_needed) { > > vcpu->mmio_needed = false; > > - vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > > - vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > > - vcpu->run->internal.ndata = 0; > > + if (!is_guest_mode(vcpu)) { > > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > > + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > > + vcpu->run->internal.ndata = 0; > > + } > > return 0; > > } > > > > -- > > 2.34.1 > > -- Thanks, Chen Yufeng
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3712dde0bf9d..b22be88196ed 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -11874,9 +11874,11 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, */ if (ret || vcpu->mmio_needed) { vcpu->mmio_needed = false; - vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; - vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; - vcpu->run->internal.ndata = 0; + if (!is_guest_mode(vcpu)) { + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; + vcpu->run->internal.ndata = 0; + } return 0; }
This patch prevents that emulation failures which result from emulating task switch for an L2-Guest results in being reported to userspace. Without this patch a malicious L2-Guest would be able to kill the L1 by triggering a race-condition between an vmexit and the task switch emulator. This patch is smiliar to commit fc3a9157d314 ("KVM: X86: Don't report L2 emulation failures to user-space") Fixes: 1051778f6e1e ("KVM: x86: Handle emulation failure directly in kvm_task_switch()") Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn> --- arch/x86/kvm/x86.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)