| Message ID | 20241203090045.942078-13-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
On 12/3/24 03:00, Nikunj A Dadhania wrote: > SecureTSC enabled guests should use TSC as the only clock source, terminate > the guest with appropriate code when clock source switches to hypervisor > controlled kvmclock. > > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/include/asm/sev-common.h | 1 + > arch/x86/include/asm/sev.h | 2 ++ > arch/x86/coco/sev/shared.c | 3 +-- > arch/x86/kernel/kvmclock.c | 9 +++++++++ > 4 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h > index 6ef92432a5ce..ad0743800b0e 100644 > --- a/arch/x86/include/asm/sev-common.h > +++ b/arch/x86/include/asm/sev-common.h > @@ -207,6 +207,7 @@ struct snp_psc_desc { > #define GHCB_TERM_SVSM_VMPL0 8 /* SVSM is present but has set VMPL to 0 */ > #define GHCB_TERM_SVSM_CAA 9 /* SVSM is present but CAA is not page aligned */ > #define GHCB_TERM_SECURE_TSC 10 /* Secure TSC initialization failed */ > +#define GHCB_TERM_SECURE_TSC_KVMCLOCK 11 /* KVM clock selected instead of Secure TSC */ > > #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index c4dca06b3b01..12b167fd6475 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -494,6 +494,7 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req > > void __init snp_secure_tsc_prepare(void); > void __init snp_secure_tsc_init(void); > +void __noreturn sev_es_terminate(unsigned int set, unsigned int reason); > > #else /* !CONFIG_AMD_MEM_ENCRYPT */ > > @@ -538,6 +539,7 @@ static inline int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_ > struct snp_guest_request_ioctl *rio) { return -ENODEV; } > static inline void __init snp_secure_tsc_prepare(void) { } > static inline void __init snp_secure_tsc_init(void) { } > +static inline void sev_es_terminate(unsigned int set, unsigned int reason) { } > > #endif /* CONFIG_AMD_MEM_ENCRYPT */ > > diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c > index 879ab48b705c..840149556241 100644 > --- a/arch/x86/coco/sev/shared.c > +++ b/arch/x86/coco/sev/shared.c > @@ -117,8 +117,7 @@ static bool __init sev_es_check_cpu_features(void) > return true; > } > > -static void __head __noreturn > -sev_es_terminate(unsigned int set, unsigned int reason) > +void __head __noreturn sev_es_terminate(unsigned int set, unsigned int reason) > { > u64 val = GHCB_MSR_TERM_REQ; > > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c > index 5b2c15214a6b..39dda04b5ba0 100644 > --- a/arch/x86/kernel/kvmclock.c > +++ b/arch/x86/kernel/kvmclock.c > @@ -21,6 +21,7 @@ > #include <asm/hypervisor.h> > #include <asm/x86_init.h> > #include <asm/kvmclock.h> > +#include <asm/sev.h> > > static int kvmclock __initdata = 1; > static int kvmclock_vsyscall __initdata = 1; > @@ -150,6 +151,14 @@ bool kvm_check_and_clear_guest_paused(void) > > static int kvm_cs_enable(struct clocksource *cs) > { > + /* > + * For a guest with SecureTSC enabled, the TSC should be the only clock > + * source. Abort the guest when kvmclock is selected as the clock > + * source. > + */ > + if (WARN_ON(cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))) > + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECURE_TSC_KVMCLOCK); > + > vclocks_set_used(VDSO_CLOCKMODE_PVCLOCK); > return 0; > }
On Tue, Dec 03, 2024 at 02:30:44PM +0530, Nikunj A Dadhania wrote: > SecureTSC enabled guests should use TSC as the only clock source, terminate > the guest with appropriate code when clock source switches to hypervisor > controlled kvmclock. This looks silly. Why not prevent kvm_register_clock() from succeeding simply on a secure-TSC guest?
On 12/30/2024 10:34 PM, Borislav Petkov wrote: > On Tue, Dec 03, 2024 at 02:30:44PM +0530, Nikunj A Dadhania wrote: >> SecureTSC enabled guests should use TSC as the only clock source, terminate >> the guest with appropriate code when clock source switches to hypervisor >> controlled kvmclock. > > This looks silly. I can drop this patch, and if the admin wants to change the clock source to kvm-clock from Secure TSC, that will be permitted. > Why not prevent kvm_register_clock() from succeeding simply > on a secure-TSC guest? Or we can have something like below that will prevent kvm-clock registration: diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c index 39dda04b5ba0..e68683a65018 100644 --- a/arch/x86/kernel/kvmclock.c +++ b/arch/x86/kernel/kvmclock.c @@ -151,14 +151,6 @@ bool kvm_check_and_clear_guest_paused(void) static int kvm_cs_enable(struct clocksource *cs) { - /* - * For a guest with SecureTSC enabled, the TSC should be the only clock - * source. Abort the guest when kvmclock is selected as the clock - * source. - */ - if (WARN_ON(cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))) - sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECURE_TSC_KVMCLOCK); - vclocks_set_used(VDSO_CLOCKMODE_PVCLOCK); return 0; } @@ -352,6 +344,12 @@ void __init kvmclock_init(void) boot_cpu_has(X86_FEATURE_NONSTOP_TSC) && !check_tsc_unstable()) kvm_clock.rating = 299; + /* + * For a guest with SecureTSC enabled, the TSC clock source should be + * used. Prevent the kvm-clock source registration. + */ + if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC)) + return; clocksource_register_hz(&kvm_clock, NSEC_PER_SEC); pv_info.name = "KVM";
On Wed, Jan 01, 2025 at 03:14:12PM +0530, Nikunj A. Dadhania wrote: > I can drop this patch, and if the admin wants to change the clock > source to kvm-clock from Secure TSC, that will be permitted. Why would the admin want that and why would we even support that? Your commit message starts with: "SecureTSC enabled guests should use TSC as the only clock source, terminate the guest with appropriate code when clock source switches to hypervisor controlled kvmclock." So what are we even doing here? You do notice that you're missing answering the "why" question on all those, which is kinda the most important one. You're adding support for something but the supported use cases are not really clear...
On 1/1/2025 9:49 PM, Borislav Petkov wrote: > On Wed, Jan 01, 2025 at 03:14:12PM +0530, Nikunj A. Dadhania wrote: >> I can drop this patch, and if the admin wants to change the clock >> source to kvm-clock from Secure TSC, that will be permitted. > > Why would the admin want that and why would we even support that? I am not saying that admin will do that, I am saying that it is a possibility. Changing clocksource is supported via sysfs interface: echo "kvm-clock" > /sys/devices/system/clocksource/clocksource0/current_clocksource This is what we are trying to prevent in this patch. Regards, Nikunj
On Thu, Jan 02, 2025 at 11:04:21AM +0530, Nikunj A. Dadhania wrote: > > > On 1/1/2025 9:49 PM, Borislav Petkov wrote: > > On Wed, Jan 01, 2025 at 03:14:12PM +0530, Nikunj A. Dadhania wrote: > >> I can drop this patch, and if the admin wants to change the clock > >> source to kvm-clock from Secure TSC, that will be permitted. > > > > Why would the admin want that and why would we even support that? > > I am not saying that admin will do that, I am saying that it is a possibility. > > Changing clocksource is supported via sysfs interface: > > echo "kvm-clock" > /sys/devices/system/clocksource/clocksource0/current_clocksource You can do that in the guest, right? Not on the host. If so, are you basically saying that users will be able to kill their guests simply by switching the clocksource? Because this would be dumb of us. And then the real thing to do should be something along the lines of "You're running a Secure TSC guest, changing the clocksource is not allowed!" or even better we warn when the user changes it but allow the change and taint the kernel. ... Yeah, stuff like that comes out only when I scratch the surface and ask about it. And then people complain about it taking too long. Well, tough luck.
On 1/2/2025 2:55 PM, Borislav Petkov wrote: > On Thu, Jan 02, 2025 at 11:04:21AM +0530, Nikunj A. Dadhania wrote: >> >> >> On 1/1/2025 9:49 PM, Borislav Petkov wrote: >>> On Wed, Jan 01, 2025 at 03:14:12PM +0530, Nikunj A. Dadhania wrote: >>>> I can drop this patch, and if the admin wants to change the clock >>>> source to kvm-clock from Secure TSC, that will be permitted. >>> >>> Why would the admin want that and why would we even support that? >> >> I am not saying that admin will do that, I am saying that it is a possibility. >> >> Changing clocksource is supported via sysfs interface: >> >> echo "kvm-clock" > /sys/devices/system/clocksource/clocksource0/current_clocksource > > You can do that in the guest, right? Yes. > > Not on the host. Right, kvm-clock is not available on host. > If so, are you basically saying that users will be able to kill their guests > simply by switching the clocksource? > > Because this would be dumb of us. > > And then the real thing to do should be something along the lines of > > "You're running a Secure TSC guest, changing the clocksource is not allowed!" > > or even better we warn when the user changes it but allow the change and taint > the kernel. Sure, that sounds better. I will keep the warning and taint the kernel. Regards, Nikunj
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index 6ef92432a5ce..ad0743800b0e 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -207,6 +207,7 @@ struct snp_psc_desc { #define GHCB_TERM_SVSM_VMPL0 8 /* SVSM is present but has set VMPL to 0 */ #define GHCB_TERM_SVSM_CAA 9 /* SVSM is present but CAA is not page aligned */ #define GHCB_TERM_SECURE_TSC 10 /* Secure TSC initialization failed */ +#define GHCB_TERM_SECURE_TSC_KVMCLOCK 11 /* KVM clock selected instead of Secure TSC */ #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index c4dca06b3b01..12b167fd6475 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -494,6 +494,7 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req void __init snp_secure_tsc_prepare(void); void __init snp_secure_tsc_init(void); +void __noreturn sev_es_terminate(unsigned int set, unsigned int reason); #else /* !CONFIG_AMD_MEM_ENCRYPT */ @@ -538,6 +539,7 @@ static inline int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_ struct snp_guest_request_ioctl *rio) { return -ENODEV; } static inline void __init snp_secure_tsc_prepare(void) { } static inline void __init snp_secure_tsc_init(void) { } +static inline void sev_es_terminate(unsigned int set, unsigned int reason) { } #endif /* CONFIG_AMD_MEM_ENCRYPT */ diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c index 879ab48b705c..840149556241 100644 --- a/arch/x86/coco/sev/shared.c +++ b/arch/x86/coco/sev/shared.c @@ -117,8 +117,7 @@ static bool __init sev_es_check_cpu_features(void) return true; } -static void __head __noreturn -sev_es_terminate(unsigned int set, unsigned int reason) +void __head __noreturn sev_es_terminate(unsigned int set, unsigned int reason) { u64 val = GHCB_MSR_TERM_REQ; diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c index 5b2c15214a6b..39dda04b5ba0 100644 --- a/arch/x86/kernel/kvmclock.c +++ b/arch/x86/kernel/kvmclock.c @@ -21,6 +21,7 @@ #include <asm/hypervisor.h> #include <asm/x86_init.h> #include <asm/kvmclock.h> +#include <asm/sev.h> static int kvmclock __initdata = 1; static int kvmclock_vsyscall __initdata = 1; @@ -150,6 +151,14 @@ bool kvm_check_and_clear_guest_paused(void) static int kvm_cs_enable(struct clocksource *cs) { + /* + * For a guest with SecureTSC enabled, the TSC should be the only clock + * source. Abort the guest when kvmclock is selected as the clock + * source. + */ + if (WARN_ON(cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))) + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECURE_TSC_KVMCLOCK); + vclocks_set_used(VDSO_CLOCKMODE_PVCLOCK); return 0; }
SecureTSC enabled guests should use TSC as the only clock source, terminate the guest with appropriate code when clock source switches to hypervisor controlled kvmclock. Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> --- arch/x86/include/asm/sev-common.h | 1 + arch/x86/include/asm/sev.h | 2 ++ arch/x86/coco/sev/shared.c | 3 +-- arch/x86/kernel/kvmclock.c | 9 +++++++++ 4 files changed, 13 insertions(+), 2 deletions(-)