diff mbox series

[v6,12/16] x86/sev: Prevent RDTSC/RDTSCP interception for Secure TSC enabled guests

Message ID 20231128125959.1810039-13-nikunj@amd.com
State New
Headers
Series Add Secure TSC support for SNP guests |

Commit Message

Nikunj A. Dadhania Nov. 28, 2023, 12:59 p.m. UTC 
  The hypervisor should not be intercepting RDTSC/RDTSCP when Secure TSC
is enabled. A #VC exception will be generated if the RDTSC/RDTSCP
instructions are being intercepted. If this should occur and Secure
TSC is enabled, terminate guest execution.

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/kernel/sev-shared.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Dionna Amalie Glaze Dec. 5, 2023, 5:16 p.m. UTC | #1 
On Tue, Nov 28, 2023 at 5:02 AM Nikunj A Dadhania <nikunj@amd.com> wrote:
>
> The hypervisor should not be intercepting RDTSC/RDTSCP when Secure TSC
> is enabled. A #VC exception will be generated if the RDTSC/RDTSCP
> instructions are being intercepted. If this should occur and Secure
> TSC is enabled, terminate guest execution.
>
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>  arch/x86/kernel/sev-shared.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index ccb0915e84e1..6d9ef5897421 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -991,6 +991,16 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb,
>         bool rdtscp = (exit_code == SVM_EXIT_RDTSCP);
>         enum es_result ret;
>
> +       /*
> +        * RDTSC and RDTSCP should not be intercepted when Secure TSC is
> +        * enabled. Terminate the SNP guest when the interception is enabled.
> +        * This file is included from kernel/sev.c and boot/compressed/sev.c,
> +        * use sev_status here as cc_platform_has() is not available when
> +        * compiling boot/compressed/sev.c.
> +        */
> +       if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
> +               return ES_VMM_ERROR;

Is this not a cc_platform_has situation? I don't recall how the
conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE
versus having a cc_attr_secure_tsc

> +
>         ret = sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, 0, 0);
>         if (ret != ES_OK)
>                 return ret;
> --
> 2.34.1
>
Nikunj A. Dadhania Dec. 6, 2023, 4:37 a.m. UTC | #2 
On 12/5/2023 10:46 PM, Dionna Amalie Glaze wrote:
> On Tue, Nov 28, 2023 at 5:02 AM Nikunj A Dadhania <nikunj@amd.com> wrote:
>>
>> The hypervisor should not be intercepting RDTSC/RDTSCP when Secure TSC
>> is enabled. A #VC exception will be generated if the RDTSC/RDTSCP
>> instructions are being intercepted. If this should occur and Secure
>> TSC is enabled, terminate guest execution.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>  arch/x86/kernel/sev-shared.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
>> index ccb0915e84e1..6d9ef5897421 100644
>> --- a/arch/x86/kernel/sev-shared.c
>> +++ b/arch/x86/kernel/sev-shared.c
>> @@ -991,6 +991,16 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb,
>>         bool rdtscp = (exit_code == SVM_EXIT_RDTSCP);
>>         enum es_result ret;
>>
>> +       /*
>> +        * RDTSC and RDTSCP should not be intercepted when Secure TSC is
>> +        * enabled. Terminate the SNP guest when the interception is enabled.
>> +        * This file is included from kernel/sev.c and boot/compressed/sev.c,
>> +        * use sev_status here as cc_platform_has() is not available when
>> +        * compiling boot/compressed/sev.c.
>> +        */
>> +       if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
>> +               return ES_VMM_ERROR;
> 
> Is this not a cc_platform_has situation? I don't recall how the
> conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE
> versus having a cc_attr_secure_tsc

For SNP, SecureTSC is an opt-in feature. AFAIU, for TDX the feature is 
turned on by default. So SNP guests need to check if the VMM has enabled 
the feature before moving forward with SecureTSC initializations.

The idea was to have some generic name instead of AMD specific SecureTSC
(cc_attr_secure_tsc), and I had sought comments from Kirill [1]. After 
that discussion I have added a synthetic flag for Secure TSC[2].

Regards
Nikunj

1. https://lore.kernel.org/lkml/55de810b-66f9-49e3-8459-b7cac1532a0c@amd.com/
2. https://lore.kernel.org/lkml/20231128125959.1810039-10-nikunj@amd.com/
Dionna Amalie Glaze Dec. 6, 2023, 6:45 p.m. UTC | #3 
> >> +       if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
> >> +               return ES_VMM_ERROR;
> >
> > Is this not a cc_platform_has situation? I don't recall how the
> > conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE
> > versus having a cc_attr_secure_tsc
>
> For SNP, SecureTSC is an opt-in feature. AFAIU, for TDX the feature is
> turned on by default. So SNP guests need to check if the VMM has enabled
> the feature before moving forward with SecureTSC initializations.
>
> The idea was to have some generic name instead of AMD specific SecureTSC
> (cc_attr_secure_tsc), and I had sought comments from Kirill [1]. After
> that discussion I have added a synthetic flag for Secure TSC[2].
>

So with regards to [2], this sev_status flag check should be
cpu_has_feature(X86_FEATURE_SNP_SECURE_TSC)? I'm not sure if that's
available in early boot where this code is used, so if it isn't,
probably that's worth a comment.
Nikunj A. Dadhania Dec. 7, 2023, 6:12 a.m. UTC | #4 
On 12/7/2023 12:15 AM, Dionna Amalie Glaze wrote:
>>>> +       if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
>>>> +               return ES_VMM_ERROR;
>>>
>>> Is this not a cc_platform_has situation? I don't recall how the
>>> conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE
>>> versus having a cc_attr_secure_tsc
>>
>> For SNP, SecureTSC is an opt-in feature. AFAIU, for TDX the feature is
>> turned on by default. So SNP guests need to check if the VMM has enabled
>> the feature before moving forward with SecureTSC initializations.
>>
>> The idea was to have some generic name instead of AMD specific SecureTSC
>> (cc_attr_secure_tsc), and I had sought comments from Kirill [1]. After
>> that discussion I have added a synthetic flag for Secure TSC[2].
>>
> 
> So with regards to [2], this sev_status flag check should be
> cpu_has_feature(X86_FEATURE_SNP_SECURE_TSC)? I'm not sure if that's
> available in early boot where this code is used, so if it isn't,
> probably that's worth a comment.

Right, I will update the comment.

Regards
Nikunj
diff mbox series

Patch

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index ccb0915e84e1..6d9ef5897421 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -991,6 +991,16 @@  static enum es_result vc_handle_rdtsc(struct ghcb *ghcb,
 	bool rdtscp = (exit_code == SVM_EXIT_RDTSCP);
 	enum es_result ret;
 
+	/*
+	 * RDTSC and RDTSCP should not be intercepted when Secure TSC is
+	 * enabled. Terminate the SNP guest when the interception is enabled.
+	 * This file is included from kernel/sev.c and boot/compressed/sev.c,
+	 * use sev_status here as cc_platform_has() is not available when
+	 * compiling boot/compressed/sev.c.
+	 */
+	if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
+		return ES_VMM_ERROR;
+
 	ret = sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, 0, 0);
 	if (ret != ES_OK)
 		return ret;