mbox series

[0/9] nSVM: Security and correctness fixes

Message ID 20221103135736.42295-1-mlevitsk@redhat.com (mailing list archive)
Headers show
Series nSVM: Security and correctness fixes | expand

Message

Maxim Levitsky Nov. 3, 2022, 1:57 p.m. UTC
Recently while trying to fix some unit tests I found a CVE in SVM nested code.

In 'shutdown_interception' vmexit handler we call kvm_vcpu_reset.

However if running nested and L1 doesn't intercept shutdown, we will still end
up running this function and trigger a bug in it.

The bug is that this function resets the 'vcpu->arch.hflags' without properly
leaving the nested state, which leaves the vCPU in inconsistent state, which
later triggers a kernel panic in SVM code.

The same bug can likely be triggered by sending INIT via local apic to a vCPU
which runs a nested guest.

On VMX we are lucky that the issue can't happen because VMX always intercepts
triple faults, thus triple fault in L2 will always be redirected to L1.
Plus the 'handle_triple_fault' of VMX doesn't reset the vCPU.

INIT IPI can't happen on VMX either because INIT events are masked while in
VMX mode.

First 4 patches in this series address the above issue, and are
already posted on the list with title,
('nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept')
I addressed the review feedback and also added a unit test to hit this issue.

In addition to these patches I noticed that KVM doesn't honour SHUTDOWN intercept bit
of L1 on SVM, and I included a fix to do so - its only for correctness
as a normal hypervisor should always intercept SHUTDOWN.
A unit test on the other hand might want to not do so.
I also extendted the triple_fault_test selftest to hit this issue.

Finaly I found another security issue, I found a way to
trigger a kernel non rate limited printk on SVM from the guest, and
last patch in the series fixes that.

A unit test I posted to kvm-unit-tests project hits this issue, so
no selftest was added.

Best regards,
	Maxim Levitsky

Maxim Levitsky (9):
  KVM: x86: nSVM: leave nested mode on vCPU free
  KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while
    still in use
  KVM: x86: add kvm_leave_nested
  KVM: x86: forcibly leave nested mode on vCPU reset
  KVM: selftests: move idt_entry to header
  kvm: selftests: add svm nested shutdown test
  KVM: x86: allow L1 to not intercept triple fault
  KVM: selftests: add svm part to triple_fault_test
  KVM: x86: remove exit_int_info warning in svm_handle_exit

 arch/x86/kvm/svm/nested.c                     | 12 +++-
 arch/x86/kvm/svm/svm.c                        | 10 +--
 arch/x86/kvm/vmx/nested.c                     |  4 +-
 arch/x86/kvm/x86.c                            | 29 ++++++--
 tools/testing/selftests/kvm/.gitignore        |  1 +
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../selftests/kvm/include/x86_64/processor.h  | 13 ++++
 .../selftests/kvm/lib/x86_64/processor.c      | 13 ----
 .../kvm/x86_64/svm_nested_shutdown_test.c     | 71 +++++++++++++++++++
 .../kvm/x86_64/triple_fault_event_test.c      | 71 ++++++++++++++-----
 10 files changed, 174 insertions(+), 51 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/svm_nested_shutdown_test.c

Comments

Maxim Levitsky Nov. 3, 2022, 2:06 p.m. UTC | #1
On Thu, 2022-11-03 at 15:57 +0200, Maxim Levitsky wrote:
> Recently while trying to fix some unit tests I found a CVE in SVM nested code.
> 
> In 'shutdown_interception' vmexit handler we call kvm_vcpu_reset.
> 
> However if running nested and L1 doesn't intercept shutdown, we will still end
> up running this function and trigger a bug in it.
> 
> The bug is that this function resets the 'vcpu->arch.hflags' without properly
> leaving the nested state, which leaves the vCPU in inconsistent state, which
> later triggers a kernel panic in SVM code.
> 
> The same bug can likely be triggered by sending INIT via local apic to a vCPU
> which runs a nested guest.
> 
> On VMX we are lucky that the issue can't happen because VMX always intercepts
> triple faults, thus triple fault in L2 will always be redirected to L1.
> Plus the 'handle_triple_fault' of VMX doesn't reset the vCPU.
> 
> INIT IPI can't happen on VMX either because INIT events are masked while in
> VMX mode.
> 
> First 4 patches in this series address the above issue, and are
> already posted on the list with title,
> ('nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept')
> I addressed the review feedback and also added a unit test to hit this issue.
> 
> In addition to these patches I noticed that KVM doesn't honour SHUTDOWN intercept bit
> of L1 on SVM, and I included a fix to do so - its only for correctness
> as a normal hypervisor should always intercept SHUTDOWN.
> A unit test on the other hand might want to not do so.
> I also extendted the triple_fault_test selftest to hit this issue.
> 
> Finaly I found another security issue, I found a way to
> trigger a kernel non rate limited printk on SVM from the guest, and
> last patch in the series fixes that.
> 
> A unit test I posted to kvm-unit-tests project hits this issue, so
> no selftest was added.
> 
> Best regards,
>         Maxim Levitsky
> 
> Maxim Levitsky (9):
>   KVM: x86: nSVM: leave nested mode on vCPU free
>   KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while
>     still in use
>   KVM: x86: add kvm_leave_nested
>   KVM: x86: forcibly leave nested mode on vCPU reset
>   KVM: selftests: move idt_entry to header
>   kvm: selftests: add svm nested shutdown test
>   KVM: x86: allow L1 to not intercept triple fault
>   KVM: selftests: add svm part to triple_fault_test
>   KVM: x86: remove exit_int_info warning in svm_handle_exit
> 
>  arch/x86/kvm/svm/nested.c                     | 12 +++-
>  arch/x86/kvm/svm/svm.c                        | 10 +--
>  arch/x86/kvm/vmx/nested.c                     |  4 +-
>  arch/x86/kvm/x86.c                            | 29 ++++++--
>  tools/testing/selftests/kvm/.gitignore        |  1 +
>  tools/testing/selftests/kvm/Makefile          |  1 +
>  .../selftests/kvm/include/x86_64/processor.h  | 13 ++++
>  .../selftests/kvm/lib/x86_64/processor.c      | 13 ----
>  .../kvm/x86_64/svm_nested_shutdown_test.c     | 71 +++++++++++++++++++
>  .../kvm/x86_64/triple_fault_event_test.c      | 71 ++++++++++++++-----
>  10 files changed, 174 insertions(+), 51 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/svm_nested_shutdown_test.c
> 
> -- 
> 2.34.3
> 
> 


I jumped the gun a bit with this patch series, there are few checkpatch.pl issues and some
leftovers I didn't remove. I'll resend this shortly.

Best regards,
	Maxim Levitsky