| Message ID | 20230110171845.20542-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Protection Key Supervisor support | expand |
On 10.01.2023 18:18, Andrew Cooper wrote: > While the reset value of the register is 0, it might not be after kexec/etc. > If PKEY0.{WD,AD} have leaked in from an earlier context, construction of a PV > dom0 will explode. > > Sequencing wise, this must come after setting CR4.PKE, and before we touch any > user mappings. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > For sequencing, it could also come after setting XCR0.PKRU too, but then we'd > need to construct an empty XSAVE area to XRSTOR from, and that would be even > more horrible to arrange. That would be ugly for other reasons as well, I think. > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -936,6 +936,9 @@ void cpu_init(void) > write_debugreg(6, X86_DR6_DEFAULT); > write_debugreg(7, X86_DR7_DEFAULT); > > + if (cpu_has_pku) > + wrpkru(0); What about the BSP during S3 resume? Shouldn't we play safe there too, just in case? Jan
On 12/01/2023 12:47 pm, Jan Beulich wrote: > On 10.01.2023 18:18, Andrew Cooper wrote: >> While the reset value of the register is 0, it might not be after kexec/etc. >> If PKEY0.{WD,AD} have leaked in from an earlier context, construction of a PV >> dom0 will explode. >> >> Sequencing wise, this must come after setting CR4.PKE, and before we touch any >> user mappings. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> >> For sequencing, it could also come after setting XCR0.PKRU too, but then we'd >> need to construct an empty XSAVE area to XRSTOR from, and that would be even >> more horrible to arrange. > That would be ugly for other reasons as well, I think. Yeah - I absolutely don't want to go down this route. > >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -936,6 +936,9 @@ void cpu_init(void) >> write_debugreg(6, X86_DR6_DEFAULT); >> write_debugreg(7, X86_DR7_DEFAULT); >> >> + if (cpu_has_pku) >> + wrpkru(0); > What about the BSP during S3 resume? Shouldn't we play safe there too, just > in case? Out of S3, I think it's reasonable to rely on proper reset values, and for pkru, and any issues of it being "wrong" should be fixed when we reload d0v0's XSAVE state. That said, I'm wanting to try and merge parts of the boot and S3 paths because we're finding no end of errors/oversights, not least because we have no automated testing of S3 suspend/resume. Servers typically don't implement it, and fixes either come from code inspection, or Qubes noticing (which is absolutely better than nothing, but not a great reflection on Xen). But to merge these things, I first need to finish the work to make microcode loading properly early, and then fix up some of the feature detection paths, and cleanly separate feature detection from applying the chosen configuration, at which point I hope the latter part will be reusable on the S3 resume path. I don't expect this work to happen imminently... ~Andrew
On 12.01.2023 18:07, Andrew Cooper wrote: > On 12/01/2023 12:47 pm, Jan Beulich wrote: >> On 10.01.2023 18:18, Andrew Cooper wrote: >>> --- a/xen/arch/x86/cpu/common.c >>> +++ b/xen/arch/x86/cpu/common.c >>> @@ -936,6 +936,9 @@ void cpu_init(void) >>> write_debugreg(6, X86_DR6_DEFAULT); >>> write_debugreg(7, X86_DR7_DEFAULT); >>> >>> + if (cpu_has_pku) >>> + wrpkru(0); >> What about the BSP during S3 resume? Shouldn't we play safe there too, just >> in case? > > Out of S3, I think it's reasonable to rely on proper reset values, and > for pkru, and any issues of it being "wrong" should be fixed when we > reload d0v0's XSAVE state. Fair enough: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 0412dbc915e5..fe92f29c2dc6 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -936,6 +936,9 @@ void cpu_init(void) write_debugreg(6, X86_DR6_DEFAULT); write_debugreg(7, X86_DR7_DEFAULT); + if (cpu_has_pku) + wrpkru(0); + /* * If the platform is performing a Secure Launch via SKINIT, GIF is * clear to prevent external interrupts interfering with Secure diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h index a3ad9ebee4e9..044cfd9f882d 100644 --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -109,6 +109,7 @@ /* CPUID level 0x00000007:0.ecx */ #define cpu_has_avx512_vbmi boot_cpu_has(X86_FEATURE_AVX512_VBMI) +#define cpu_has_pku boot_cpu_has(X86_FEATURE_PKU) #define cpu_has_avx512_vbmi2 boot_cpu_has(X86_FEATURE_AVX512_VBMI2) #define cpu_has_gfni boot_cpu_has(X86_FEATURE_GFNI) #define cpu_has_vaes boot_cpu_has(X86_FEATURE_VAES) diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 566422600d94..6deadcf74763 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1798,7 +1798,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( boot_cpu_has(X86_FEATURE_FSGSBASE) ) set_in_cr4(X86_CR4_FSGSBASE); - if ( boot_cpu_has(X86_FEATURE_PKU) ) + if ( cpu_has_pku ) set_in_cr4(X86_CR4_PKE); if ( opt_invpcid && cpu_has_invpcid )
While the reset value of the register is 0, it might not be after kexec/etc. If PKEY0.{WD,AD} have leaked in from an earlier context, construction of a PV dom0 will explode. Sequencing wise, this must come after setting CR4.PKE, and before we touch any user mappings. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> For sequencing, it could also come after setting XCR0.PKRU too, but then we'd need to construct an empty XSAVE area to XRSTOR from, and that would be even more horrible to arrange. --- xen/arch/x86/cpu/common.c | 3 +++ xen/arch/x86/include/asm/cpufeature.h | 1 + xen/arch/x86/setup.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-)