diff mbox series

[7/9] x86/sev-es: Include XSS value in GHCB CPUID request

Message ID 20231010200220.897953-8-john.allen@amd.com
State New
Headers
Series SVM guest shadow stack support |

Commit Message

John Allen Oct. 10, 2023, 8:02 p.m. UTC 
  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(+)

Comments

Borislav Petkov Oct. 12, 2023, 12:59 p.m. UTC | #1 
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. :)
John Allen Oct. 17, 2023, 6:12 p.m. UTC | #2 
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
Borislav Petkov Oct. 17, 2023, 6:49 p.m. UTC | #3 
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.
Maxim Levitsky Nov. 2, 2023, 6:14 p.m. UTC | #4 
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 mbox series

Patch

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;