| Message ID | 20221209043804.942352-3-aik@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: Enable AMD SEV-ES DebugSwap | expand |
On Fri, Dec 09, 2022 at 03:38:03PM +1100, Alexey Kardashevskiy wrote: > AMD Milan (Fam 19h) introduces support for the swapping, as type 'B', "type B" means nothing to people who don't have an intimate APM knowledge. Let's try again, this time with a more accessible formulation: "The debug registers are handled a bit differently when doing a world switch of a SEV-ES guest: the guest debug registers values are saved and restored as usual and as one would expect. The *host* debug registers are not saved to the host save area so if the host is doing any debug activity, that host should take care to stash its debug registers values into the host save area before running guests. See Table B-3. Swap Types and the AMD APM volume 2." And now you can go into detail explaining which regs exactly and so on. > of DR[0-3] and DR[0-3]_ADDR_MASK registers. Software enables this by > setting SEV_FEATURES[5] (called "DebugSwap") in the VMSA which makes > data breakpoints work in SEV-ES VMs. > > For type 'B' swaps the hardware saves/restores the VM state on > VMEXIT/VMRUN in VMSA, and restores the host state on VMEXIT. Yeah, close but I'd prefer a more detailed explanation and a reference to the APM so that people can follow and read more info if needed. > > Enable DebugSwap in VMSA but only if CPUID Fn80000021_EAX[0] > ("NoNestedDataBp", "Processor ignores nested data breakpoints") is > supported by the SOC as otherwise a malicious guest can cause > the infinite #DB loop DoS. > > Save DR[0-3] / DR[0-3]_ADDR_MASK in the host save area before VMRUN > as type 'B' swap does not do this part. > > Eliminate DR7 and #DB intercepts as: > - they are not needed when DebugSwap is supported; > - #VC for these intercepts is most likely not supported anyway and > kills the VM. > Keep DR7 intercepted unless DebugSwap enabled to prevent > the infinite #DB loop DoS. > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > --- > Changes: > v2: > * debug_swap moved from vcpu to module_param > * rewrote commit log > > --- > > "DR7 access must remain intercepted for an SEV-ES guest" - I could not > figure out the exact reasoning why it is there in the first place, > IIUC this is to prevent loop of #DBs in the VM. Let's ask Mr. Lendacky: 8d4846b9b150 ("KVM: SVM: Prevent debugging under SEV-ES") > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index efaaef2b7ae1..800ea2a778cc 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -21,6 +21,7 @@ > #include <asm/pkru.h> > #include <asm/trapnr.h> > #include <asm/fpu/xcr.h> > +#include <asm/debugreg.h> > > #include "mmu.h" > #include "x86.h" > @@ -52,11 +53,21 @@ module_param_named(sev, sev_enabled, bool, 0444); > /* enable/disable SEV-ES support */ > static bool sev_es_enabled = true; > module_param_named(sev_es, sev_es_enabled, bool, 0444); > + > +/* enable/disable SEV-ES DebugSwap support */ > +static bool sev_es_debug_swap_enabled = true; > +module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0644); > #else > #define sev_enabled false > #define sev_es_enabled false > +#define sev_es_debug_swap false > #endif /* CONFIG_KVM_AMD_SEV */ > > +bool sev_es_is_debug_swap_enabled(void) > +{ > + return sev_es_debug_swap_enabled; > +} > + > static u8 sev_enc_bit; > static DECLARE_RWSEM(sev_deactivate_lock); > static DEFINE_MUTEX(sev_bitmap_lock); > @@ -604,6 +615,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) > save->xss = svm->vcpu.arch.ia32_xss; > save->dr6 = svm->vcpu.arch.dr6; > > + if (sev_es_is_debug_swap_enabled()) > + save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP; > + > pr_debug("Virtual Machine Save Area (VMSA):\n"); > print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); > > @@ -2249,6 +2263,9 @@ void __init sev_hardware_setup(void) > out: > sev_enabled = sev_supported; > sev_es_enabled = sev_es_supported; > + if (sev_es_debug_swap_enabled) > + sev_es_debug_swap_enabled = sev_es_enabled && > + boot_cpu_has(X86_FEATURE_NO_NESTED_DATA_BP); check_for_deprecated_apis: WARNING: arch/x86/kvm/svm/sev.c:2268: Do not use boot_cpu_has() - use cpu_feature_enabled() instead.
On 1/10/23 12:00, Borislav Petkov wrote: > On Fri, Dec 09, 2022 at 03:38:03PM +1100, Alexey Kardashevskiy wrote: >> >> "DR7 access must remain intercepted for an SEV-ES guest" - I could not >> figure out the exact reasoning why it is there in the first place, >> IIUC this is to prevent loop of #DBs in the VM. > > Let's ask Mr. Lendacky: > > 8d4846b9b150 ("KVM: SVM: Prevent debugging under SEV-ES") The DR7 requirements were to prevent a malicious SEV-ES guest from setting up data breakpoints on the #VC IDT entry/stack and causing an infinite loop. Thanks, Tom > >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index efaaef2b7ae1..800ea2a778cc 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -21,6 +21,7 @@ >> #include <asm/pkru.h> >> #include <asm/trapnr.h> >> #include <asm/fpu/xcr.h> >> +#include <asm/debugreg.h> >> >> #include "mmu.h" >> #include "x86.h" >> @@ -52,11 +53,21 @@ module_param_named(sev, sev_enabled, bool, 0444); >> /* enable/disable SEV-ES support */ >> static bool sev_es_enabled = true; >> module_param_named(sev_es, sev_es_enabled, bool, 0444); >> + >> +/* enable/disable SEV-ES DebugSwap support */ >> +static bool sev_es_debug_swap_enabled = true; >> +module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0644); >> #else >> #define sev_enabled false >> #define sev_es_enabled false >> +#define sev_es_debug_swap false >> #endif /* CONFIG_KVM_AMD_SEV */ >> >> +bool sev_es_is_debug_swap_enabled(void) >> +{ >> + return sev_es_debug_swap_enabled; >> +} >> + >> static u8 sev_enc_bit; >> static DECLARE_RWSEM(sev_deactivate_lock); >> static DEFINE_MUTEX(sev_bitmap_lock); >> @@ -604,6 +615,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) >> save->xss = svm->vcpu.arch.ia32_xss; >> save->dr6 = svm->vcpu.arch.dr6; >> >> + if (sev_es_is_debug_swap_enabled()) >> + save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP; >> + >> pr_debug("Virtual Machine Save Area (VMSA):\n"); >> print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); >> >> @@ -2249,6 +2263,9 @@ void __init sev_hardware_setup(void) >> out: >> sev_enabled = sev_supported; >> sev_es_enabled = sev_es_supported; >> + if (sev_es_debug_swap_enabled) >> + sev_es_debug_swap_enabled = sev_es_enabled && >> + boot_cpu_has(X86_FEATURE_NO_NESTED_DATA_BP); > > check_for_deprecated_apis: WARNING: arch/x86/kvm/svm/sev.c:2268: Do not use boot_cpu_has() - use cpu_feature_enabled() instead. >
On 11/1/23 05:00, Borislav Petkov wrote: > On Fri, Dec 09, 2022 at 03:38:03PM +1100, Alexey Kardashevskiy wrote: >> AMD Milan (Fam 19h) introduces support for the swapping, as type 'B', > > "type B" means nothing to people who don't have an intimate APM knowledge. > > Let's try again, this time with a more accessible formulation: > > "The debug registers are handled a bit differently when doing a world switch of a > SEV-ES guest: the guest debug registers values are saved and restored as usual > and as one would expect. Well, SEV-ES KVM (ES == Encrypted State) does not save/restore them for the guest (well, as I would expect) as the guest registers are not visible to host to save, they are intercepted and the VM does this GHCB dance with VMGEXIT(SVM_EXIT_WRITE_DR7). > The *host* debug registers are not saved to the host save area so if the > host is doing any debug activity, that host should take care to stash its debug > registers values into the host save area before running guests. > > See Table B-3. Swap Types and the AMD APM volume 2." > > And now you can go into detail explaining which regs exactly and so on. > >> of DR[0-3] and DR[0-3]_ADDR_MASK registers. Software enables this by >> setting SEV_FEATURES[5] (called "DebugSwap") in the VMSA which makes >> data breakpoints work in SEV-ES VMs. >> >> For type 'B' swaps the hardware saves/restores the VM state on >> VMEXIT/VMRUN in VMSA, and restores the host state on VMEXIT. > > Yeah, close but I'd prefer a more detailed explanation and a reference to the > APM so that people can follow and read more info if needed. Well, the only place in APM is that "Table B-3. Swap Types and the AMD APM volume 2", and it is pretty brief, do I miss something? Thanks, > >> Enable DebugSwap in VMSA but only if CPUID Fn80000021_EAX[0] >> ("NoNestedDataBp", "Processor ignores nested data breakpoints") is >> supported by the SOC as otherwise a malicious guest can cause >> the infinite #DB loop DoS. >> >> Save DR[0-3] / DR[0-3]_ADDR_MASK in the host save area before VMRUN >> as type 'B' swap does not do this part. >> >> Eliminate DR7 and #DB intercepts as: >> - they are not needed when DebugSwap is supported; >> - #VC for these intercepts is most likely not supported anyway and >> kills the VM. >> Keep DR7 intercepted unless DebugSwap enabled to prevent >> the infinite #DB loop DoS. >> >> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> >> --- >> Changes: >> v2: >> * debug_swap moved from vcpu to module_param >> * rewrote commit log >> >> --- >> >> "DR7 access must remain intercepted for an SEV-ES guest" - I could not >> figure out the exact reasoning why it is there in the first place, >> IIUC this is to prevent loop of #DBs in the VM. > > Let's ask Mr. Lendacky: > > 8d4846b9b150 ("KVM: SVM: Prevent debugging under SEV-ES") > >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index efaaef2b7ae1..800ea2a778cc 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -21,6 +21,7 @@ >> #include <asm/pkru.h> >> #include <asm/trapnr.h> >> #include <asm/fpu/xcr.h> >> +#include <asm/debugreg.h> >> >> #include "mmu.h" >> #include "x86.h" >> @@ -52,11 +53,21 @@ module_param_named(sev, sev_enabled, bool, 0444); >> /* enable/disable SEV-ES support */ >> static bool sev_es_enabled = true; >> module_param_named(sev_es, sev_es_enabled, bool, 0444); >> + >> +/* enable/disable SEV-ES DebugSwap support */ >> +static bool sev_es_debug_swap_enabled = true; >> +module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0644); >> #else >> #define sev_enabled false >> #define sev_es_enabled false >> +#define sev_es_debug_swap false >> #endif /* CONFIG_KVM_AMD_SEV */ >> >> +bool sev_es_is_debug_swap_enabled(void) >> +{ >> + return sev_es_debug_swap_enabled; >> +} >> + >> static u8 sev_enc_bit; >> static DECLARE_RWSEM(sev_deactivate_lock); >> static DEFINE_MUTEX(sev_bitmap_lock); >> @@ -604,6 +615,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) >> save->xss = svm->vcpu.arch.ia32_xss; >> save->dr6 = svm->vcpu.arch.dr6; >> >> + if (sev_es_is_debug_swap_enabled()) >> + save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP; >> + >> pr_debug("Virtual Machine Save Area (VMSA):\n"); >> print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); >> >> @@ -2249,6 +2263,9 @@ void __init sev_hardware_setup(void) >> out: >> sev_enabled = sev_supported; >> sev_es_enabled = sev_es_supported; >> + if (sev_es_debug_swap_enabled) >> + sev_es_debug_swap_enabled = sev_es_enabled && >> + boot_cpu_has(X86_FEATURE_NO_NESTED_DATA_BP); > > check_for_deprecated_apis: WARNING: arch/x86/kvm/svm/sev.c:2268: Do not use boot_cpu_has() - use cpu_feature_enabled() instead. >
On Thu, Jan 12, 2023 at 04:45:34PM +1100, Alexey Kardashevskiy wrote: > Well, SEV-ES KVM (ES == Encrypted State) does not save/restore them for the > guest (well, as I would expect) as the guest registers are not visible to > host to save, they are intercepted and the VM does this GHCB dance with > VMGEXIT(SVM_EXIT_WRITE_DR7). But they're saved in the VMSA, as Table B-3 says. > Well, the only place in APM is that "Table B-3. Swap Types and the AMD APM > volume 2", and it is pretty brief, do I miss something? I don't understand that question - please elaborate. Thx.
On 1/12/23 05:28, Borislav Petkov wrote: > On Thu, Jan 12, 2023 at 04:45:34PM +1100, Alexey Kardashevskiy wrote: >> Well, SEV-ES KVM (ES == Encrypted State) does not save/restore them for the >> guest (well, as I would expect) as the guest registers are not visible to >> host to save, they are intercepted and the VM does this GHCB dance with >> VMGEXIT(SVM_EXIT_WRITE_DR7). > > But they're saved in the VMSA, as Table B-3 says. Correct, when this feature is enabled, the VMRUN execution will restore the guest debug registers on guest entry and save them on guest exit. Thanks, Tom > >> Well, the only place in APM is that "Table B-3. Swap Types and the AMD APM >> volume 2", and it is pretty brief, do I miss something? > > I don't understand that question - please elaborate. > > Thx. >
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index 0361626841bc..373a0edda588 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -273,6 +273,7 @@ enum avic_ipi_failure_cause { #define AVIC_HPA_MASK ~((0xFFFULL << 52) | 0xFFF) #define VMCB_AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL +#define SVM_SEV_FEAT_DEBUG_SWAP BIT(5) struct vmcb_seg { u16 selector; diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 199a2ecef1ce..0fae611abe4a 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -386,6 +386,8 @@ static inline bool vmcb12_is_intercept(struct vmcb_ctrl_area_cached *control, u3 return test_bit(bit, (unsigned long *)&control->intercepts); } +extern bool sev_es_is_debug_swap_enabled(void); + static inline void set_dr_intercepts(struct vcpu_svm *svm) { struct vmcb *vmcb = svm->vmcb01.ptr; @@ -407,8 +409,10 @@ static inline void set_dr_intercepts(struct vcpu_svm *svm) vmcb_set_intercept(&vmcb->control, INTERCEPT_DR6_WRITE); } - vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_READ); - vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_WRITE); + if (!sev_es_guest(svm->vcpu.kvm) || !sev_es_is_debug_swap_enabled()) { + vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_READ); + vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_WRITE); + } recalc_intercepts(svm); } @@ -419,8 +423,12 @@ static inline void clr_dr_intercepts(struct vcpu_svm *svm) vmcb->control.intercepts[INTERCEPT_DR] = 0; - /* DR7 access must remain intercepted for an SEV-ES guest */ - if (sev_es_guest(svm->vcpu.kvm)) { + /* + * DR7 access must remain intercepted for an SEV-ES guest unless DebugSwap + * (depends on NO_NESTED_DATA_BP) is enabled as otherwise a VM writing to DR7 + * from the #DB handler may trigger infinite loop of #DB's. + */ + if (sev_es_guest(svm->vcpu.kvm) && !sev_es_is_debug_swap_enabled()) { vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_READ); vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_WRITE); } diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index efaaef2b7ae1..800ea2a778cc 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -21,6 +21,7 @@ #include <asm/pkru.h> #include <asm/trapnr.h> #include <asm/fpu/xcr.h> +#include <asm/debugreg.h> #include "mmu.h" #include "x86.h" @@ -52,11 +53,21 @@ module_param_named(sev, sev_enabled, bool, 0444); /* enable/disable SEV-ES support */ static bool sev_es_enabled = true; module_param_named(sev_es, sev_es_enabled, bool, 0444); + +/* enable/disable SEV-ES DebugSwap support */ +static bool sev_es_debug_swap_enabled = true; +module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0644); #else #define sev_enabled false #define sev_es_enabled false +#define sev_es_debug_swap false #endif /* CONFIG_KVM_AMD_SEV */ +bool sev_es_is_debug_swap_enabled(void) +{ + return sev_es_debug_swap_enabled; +} + static u8 sev_enc_bit; static DECLARE_RWSEM(sev_deactivate_lock); static DEFINE_MUTEX(sev_bitmap_lock); @@ -604,6 +615,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) save->xss = svm->vcpu.arch.ia32_xss; save->dr6 = svm->vcpu.arch.dr6; + if (sev_es_is_debug_swap_enabled()) + save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP; + pr_debug("Virtual Machine Save Area (VMSA):\n"); print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); @@ -2249,6 +2263,9 @@ void __init sev_hardware_setup(void) out: sev_enabled = sev_supported; sev_es_enabled = sev_es_supported; + if (sev_es_debug_swap_enabled) + sev_es_debug_swap_enabled = sev_es_enabled && + boot_cpu_has(X86_FEATURE_NO_NESTED_DATA_BP); #endif } @@ -3027,6 +3044,18 @@ void sev_es_prepare_switch_to_guest(struct sev_es_save_area *hostsa) /* MSR_IA32_XSS is restored on VMEXIT, save the currnet host value */ hostsa->xss = host_xss; + + /* The DebugSwap SEV feature does Type B swaps of DR[0-3] */ + if (sev_es_is_debug_swap_enabled()) { + hostsa->dr0 = native_get_debugreg(0); + hostsa->dr1 = native_get_debugreg(1); + hostsa->dr2 = native_get_debugreg(2); + hostsa->dr3 = native_get_debugreg(3); + hostsa->dr0_addr_mask = amd_get_dr_addr_mask(0); + hostsa->dr1_addr_mask = amd_get_dr_addr_mask(1); + hostsa->dr2_addr_mask = amd_get_dr_addr_mask(2); + hostsa->dr3_addr_mask = amd_get_dr_addr_mask(3); + } } void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector) diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index ce362e88a567..697804d46545 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -1189,7 +1189,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu) set_exception_intercept(svm, UD_VECTOR); set_exception_intercept(svm, MC_VECTOR); set_exception_intercept(svm, AC_VECTOR); - set_exception_intercept(svm, DB_VECTOR); + if (!sev_es_is_debug_swap_enabled()) + set_exception_intercept(svm, DB_VECTOR); /* * Guest access to VMware backdoor ports could legitimately * trigger #GP because of TSS I/O permission bitmap.
AMD Milan (Fam 19h) introduces support for the swapping, as type 'B', of DR[0-3] and DR[0-3]_ADDR_MASK registers. Software enables this by setting SEV_FEATURES[5] (called "DebugSwap") in the VMSA which makes data breakpoints work in SEV-ES VMs. For type 'B' swaps the hardware saves/restores the VM state on VMEXIT/VMRUN in VMSA, and restores the host state on VMEXIT. Enable DebugSwap in VMSA but only if CPUID Fn80000021_EAX[0] ("NoNestedDataBp", "Processor ignores nested data breakpoints") is supported by the SOC as otherwise a malicious guest can cause the infinite #DB loop DoS. Save DR[0-3] / DR[0-3]_ADDR_MASK in the host save area before VMRUN as type 'B' swap does not do this part. Eliminate DR7 and #DB intercepts as: - they are not needed when DebugSwap is supported; - #VC for these intercepts is most likely not supported anyway and kills the VM. Keep DR7 intercepted unless DebugSwap enabled to prevent the infinite #DB loop DoS. Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- Changes: v2: * debug_swap moved from vcpu to module_param * rewrote commit log --- "DR7 access must remain intercepted for an SEV-ES guest" - I could not figure out the exact reasoning why it is there in the first place, IIUC this is to prevent loop of #DBs in the VM. --- Tested with: === int x; int main(int argc, char *argv[]) { x = 1; return 0; } === gcc -g a.c rsync a.out ruby-954vm:~/ ssh -t ruby-954vm 'gdb -ex "file a.out" -ex "watch x" -ex r' where ruby-954vm is a VM. With "/sys/module/kvm_amd/parameters/debug_swap = 0", gdb does not stop on the watchpoint, with "= 1" - gdb does. --- arch/x86/include/asm/svm.h | 1 + arch/x86/kvm/svm/svm.h | 16 ++++++++--- arch/x86/kvm/svm/sev.c | 29 ++++++++++++++++++++ arch/x86/kvm/svm/svm.c | 3 +- 4 files changed, 44 insertions(+), 5 deletions(-)