Kees Cook
2020-Jul-15 15:26 UTC
[PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
On Wed, Jul 15, 2020 at 11:26:38AM +0200, Joerg Roedel wrote:> Hi Kees, > > thanks for your reviews! > > On Tue, Jul 14, 2020 at 06:40:30PM -0700, Kees Cook wrote: > > Eek, no. MSR_IA32_MISC_ENABLE_XD_DISABLE needs to be cleared very early > > during CPU startup; this can't just be skipped. > > That MSR is Intel-only, right? The boot-path installed here is only used > for SEV-ES guests, running on AMD systems, so this MSR is not even > accessed during boot on those VMs.Oh, hrm, yes, that's true. If other x86 maintainers are comfortable with this, then okay. My sense is that changing the early CPU startup paths will cause trouble down the line.> The alternative is to set up exception handling prior to calling > verify_cpu, including segments, stack and IDT. Given that verify_cpu() > does not add much value to SEV-ES guests, I'd like to avoid adding this > complexity.So, going back to the requirements here ... what things in verify_cpu() can cause exceptions? AFAICT, cpuid is safely handled (i.e. it is detected and only run in a way to avoid exceptions and the MSR reads/writes are similarly bound by CPU family/id range checks). I must be missing something. :)> > > Also, is UNWIND_HINT_EMPTY needed for the new target? > > Yes, I think it is, will add it in the next version. > > Regards, > > Joerg-- Kees Cook
Joerg Roedel
2020-Jul-15 15:48 UTC
[PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
Hi Kees, as a general note: With SEV-ES the guest kernel will get #VC exceptions for events that, without SEV-ES, would just cause a #VMEXIT to the hypervisor. On Wed, Jul 15, 2020 at 08:26:14AM -0700, Kees Cook wrote:> On Wed, Jul 15, 2020 at 11:26:38AM +0200, Joerg Roedel wrote: > > That MSR is Intel-only, right? The boot-path installed here is only used > > for SEV-ES guests, running on AMD systems, so this MSR is not even > > accessed during boot on those VMs. > > Oh, hrm, yes, that's true. If other x86 maintainers are comfortable with > this, then okay. My sense is that changing the early CPU startup paths > will cause trouble down the line.The AP startup path does not change for non SEV-ES guests. But under SEV-ES everything that might cause a #VC exception must be avoided until the kernel is ready to handle them. With the current patches this happens when the AP runs in 64bit long-mode and loaded TSS and IDT. Therefore a slightly different AP boot-path is needed for SEV-ES guests.> So, going back to the requirements here ... what things in verify_cpu() > can cause exceptions? AFAICT, cpuid is safely handled (i.e. it is > detected and only run in a way to avoid exceptions and the MSR > reads/writes are similarly bound by CPU family/id range checks). I must > be missing something. :)It is actually the CPUID instructions that cause #VC exceptions. The MSRs that are accessed on AMD processors are not intercepted in the hypervisors this code has been tested on, so these will not cause #VC exceptions. Regards, Joerg
Kees Cook
2020-Jul-15 19:49 UTC
[PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
On Wed, Jul 15, 2020 at 05:48:56PM +0200, Joerg Roedel wrote:> It is actually the CPUID instructions that cause #VC exceptions. The > MSRs that are accessed on AMD processors are not intercepted in the > hypervisors this code has been tested on, so these will not cause #VC > exceptions.Aaah. I see. Thanks for the details there. So ... can you add a bunch more comments about why/when the new entry path is being used? I really don't want to accidentally discover some unrelated refactoring down the road (in months, years, unrelated to SEV, etc) starts to also skip verify_cpu() on Intel systems. There had been a lot of BIOSes that set this MSR to disable NX, and I don't want to repeat that pain: Linux must never start an Intel CPU with that MSR set. :P -- Kees Cook
Seemingly Similar Threads
- [PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
- [PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
- [PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
- [PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs
- [PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs