| Message ID | 20231010200220.897953-8-john.allen@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SVM guest shadow stack support | expand |
On Tue, Oct 10, 2023 at 08:02:18PM +0000, John Allen wrote: > When a guest issues a cpuid instruction for Fn0000000D_x0B (CetUserOffset), the > hypervisor may intercept and access the guest XSS value. For SEV-ES, this is > encrypted and needs to be included in the GHCB to be visible to the hypervisor. > The rdmsr instruction needs to be called directly as the code may be used in > early boot in which case the rdmsr wrappers should be avoided as they are > incompatible with the decompression boot phase. > > Signed-off-by: John Allen <john.allen@amd.com> > --- > arch/x86/kernel/sev-shared.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index 2eabccde94fb..e38a1d049bc1 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -890,6 +890,21 @@ static enum es_result vc_handle_cpuid(struct ghcb *ghcb, > /* xgetbv will cause #GP - use reset value for xcr0 */ > ghcb_set_xcr0(ghcb, 1); > > + if (has_cpuflag(X86_FEATURE_SHSTK) && regs->ax == 0xd && regs->cx <= 1) { > + unsigned long lo, hi; > + u64 xss; > + > + /* > + * Since vc_handle_cpuid may be used during early boot, the > + * rdmsr wrappers are incompatible and should not be used. > + * Invoke the instruction directly. > + */ > + asm volatile("rdmsr" : "=a" (lo), "=d" (hi) > + : "c" (MSR_IA32_XSS)); Does __rdmsr() work too? I know it has exception handling but a SEV-ES guest should not fault when accessing MSR_IA32_XSS anyway, especially if it has shadow stack enabled. And if it does fault, your version would explode too but __rdmsr() would be at least less code. :)
On Thu, Oct 12, 2023 at 02:59:24PM +0200, Borislav Petkov wrote: > On Tue, Oct 10, 2023 at 08:02:18PM +0000, John Allen wrote: > > When a guest issues a cpuid instruction for Fn0000000D_x0B (CetUserOffset), the > > hypervisor may intercept and access the guest XSS value. For SEV-ES, this is > > encrypted and needs to be included in the GHCB to be visible to the hypervisor. > > The rdmsr instruction needs to be called directly as the code may be used in > > early boot in which case the rdmsr wrappers should be avoided as they are > > incompatible with the decompression boot phase. > > > > Signed-off-by: John Allen <john.allen@amd.com> > > --- > > arch/x86/kernel/sev-shared.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > > index 2eabccde94fb..e38a1d049bc1 100644 > > --- a/arch/x86/kernel/sev-shared.c > > +++ b/arch/x86/kernel/sev-shared.c > > @@ -890,6 +890,21 @@ static enum es_result vc_handle_cpuid(struct ghcb *ghcb, > > /* xgetbv will cause #GP - use reset value for xcr0 */ > > ghcb_set_xcr0(ghcb, 1); > > > > + if (has_cpuflag(X86_FEATURE_SHSTK) && regs->ax == 0xd && regs->cx <= 1) { > > + unsigned long lo, hi; > > + u64 xss; > > + > > + /* > > + * Since vc_handle_cpuid may be used during early boot, the > > + * rdmsr wrappers are incompatible and should not be used. > > + * Invoke the instruction directly. > > + */ > > + asm volatile("rdmsr" : "=a" (lo), "=d" (hi) > > + : "c" (MSR_IA32_XSS)); > > Does __rdmsr() work too? > > I know it has exception handling but a SEV-ES guest should not fault > when accessing MSR_IA32_XSS anyway, especially if it has shadow stack > enabled. And if it does fault, your version would explode too but > __rdmsr() would be at least less code. :) I looked into using __rdmsr in an earlier revision of the patch, but found that it causes a build warning: ld: warning: orphan section `__ex_table' from `arch/x86/boot/compressed/sev.o' being placed in section `__ex_table' This is due to the __ex_table section not being used during decompression boot. Do you know of a way around this? Thanks, John
On Tue, Oct 17, 2023 at 01:12:30PM -0500, John Allen wrote: > I looked into using __rdmsr in an earlier revision of the patch, but > found that it causes a build warning: > > ld: warning: orphan section `__ex_table' from `arch/x86/boot/compressed/sev.o' being placed in section `__ex_table' > > This is due to the __ex_table section not being used during > decompression boot. Do you know of a way around this? Yeah, arch/x86/boot/msr.h. We did those a while ago. I guess they could be moved to asm/shared/msr.h and renamed to something that is not a "boot_" prefix. Something like rdmsr_without_any_exception_handling_and_other_gunk_don_t_you_even_think_of_adding() ... But srsly: raw_rdmsr() raw_wrmsr() should be good, as tglx suggests offlist. You can do that in one patch and prepend your set with it. Thx.
On Tue, 2023-10-17 at 20:49 +0200, Borislav Petkov wrote: > On Tue, Oct 17, 2023 at 01:12:30PM -0500, John Allen wrote: > > I looked into using __rdmsr in an earlier revision of the patch, but > > found that it causes a build warning: > > > > ld: warning: orphan section `__ex_table' from `arch/x86/boot/compressed/sev.o' being placed in section `__ex_table' > > > > This is due to the __ex_table section not being used during > > decompression boot. Do you know of a way around this? > > Yeah, arch/x86/boot/msr.h. > > We did those a while ago. I guess they could be moved to > asm/shared/msr.h and renamed to something that is not a > "boot_" prefix. > > Something like > > rdmsr_without_any_exception_handling_and_other_gunk_don_t_you_even_think_of_adding() > > ... > > But srsly: > > raw_rdmsr() > raw_wrmsr() > > should be good, as tglx suggests offlist. > > You can do that in one patch and prepend your set with it. > > Thx. > Assuming that we will actually allow the guest to read the IA32_XSS, then it is correct, but otherwise we will need to return some cached value of IA32_XSS, the same as the guest wrote last time. Or another option: KVM can cache the last value that the guest wrote to IA32_XSS (I assume that the guest can write msrs by sharing the address and value via GHCB), and then use it when it constructs the CPUID. Best regards, Maxim Levitsky
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index 2eabccde94fb..e38a1d049bc1 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -890,6 +890,21 @@ static enum es_result vc_handle_cpuid(struct ghcb *ghcb, /* xgetbv will cause #GP - use reset value for xcr0 */ ghcb_set_xcr0(ghcb, 1); + if (has_cpuflag(X86_FEATURE_SHSTK) && regs->ax == 0xd && regs->cx <= 1) { + unsigned long lo, hi; + u64 xss; + + /* + * Since vc_handle_cpuid may be used during early boot, the + * rdmsr wrappers are incompatible and should not be used. + * Invoke the instruction directly. + */ + asm volatile("rdmsr" : "=a" (lo), "=d" (hi) + : "c" (MSR_IA32_XSS)); + xss = (hi << 32) | lo; + ghcb_set_xss(ghcb, xss); + } + ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0); if (ret != ES_OK) return ret;
When a guest issues a cpuid instruction for Fn0000000D_x0B (CetUserOffset), the hypervisor may intercept and access the guest XSS value. For SEV-ES, this is encrypted and needs to be included in the GHCB to be visible to the hypervisor. The rdmsr instruction needs to be called directly as the code may be used in early boot in which case the rdmsr wrappers should be avoided as they are incompatible with the decompression boot phase. Signed-off-by: John Allen <john.allen@amd.com> --- arch/x86/kernel/sev-shared.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)