[v2] x86/sev: Improve handling of writes to intercepted TSC MSRs

[PATCH v2] x86/sev: Improve handling of writes to intercepted TSC MSRs

Posted by Nikunj A Dadhania 2 months, 3 weeks ago

From: Sean Christopherson <seanjc@google.com>

Currently, when a Secure TSC enabled SNP guest attempts to write to the
intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel response
incorrectly implies a VMM configuration error, when in fact it is the usual
VMM configuration to intercept writes to read-only MSRs, unless explicitly
documented.

Modify the intercepted TSC MSR #VC handling:
* Write to GUEST_TSC_FREQ will generate a #GP instead of terminating the
  guest
* Write to MSR_IA32_TSC will generate a #GP instead of silently ignoring it

Add a WARN_ONCE to log the incident, as well-behaved SNP guest kernels
should never attempt to write to these MSRs.

However, continue to terminate the guest when reading from intercepted
GUEST_TSC_FREQ MSR with Secure TSC enabled, as intercepted reads indicate
an improper VMM configuration for Secure TSC enabled SNP guests.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Co-developed-by: Nikunj A Dadhania <nikunj@amd.com>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/coco/sev/vc-handle.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
index faf1fce89ed4..18be9f8bd015 100644
--- a/arch/x86/coco/sev/vc-handle.c
+++ b/arch/x86/coco/sev/vc-handle.c
@@ -371,29 +371,30 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
  * executing with Secure TSC enabled, so special handling is required for
  * accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ.
  */
-static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write)
+static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write,
+						  struct es_em_ctxt  *ctxt)
 {
 	u64 tsc;
 
 	/*
-	 * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
-	 * Terminate the SNP guest when the interception is enabled.
+	 * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
+	 * return undefined values, and GUEST_TSC_FREQ is read-only. Generate
+	 * a #GP on all writes, but WARN to log a kernel bug.
 	 */
-	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
-		return ES_VMM_ERROR;
+	if (WARN_ON_ONCE(write)) {
+		ctxt->fi.vector = X86_TRAP_GP;
+		ctxt->fi.error_code = 0;
+		return ES_EXCEPTION;
+	}
 
 	/*
-	 * Writes: Writing to MSR_IA32_TSC can cause subsequent reads of the TSC
-	 *         to return undefined values, so ignore all writes.
-	 *
-	 * Reads: Reads of MSR_IA32_TSC should return the current TSC value, use
-	 *        the value returned by rdtsc_ordered().
+	 * GUEST_TSC_FREQ read should not be intercepted when Secure TSC is
+	 * enabled. Terminate the SNP guest when the interception is enabled.
 	 */
-	if (write) {
-		WARN_ONCE(1, "TSC MSR writes are verboten!\n");
-		return ES_OK;
-	}
+	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
+		return ES_VMM_ERROR;
 
+	/* Reads of MSR_IA32_TSC should return the current TSC value. */
 	tsc = rdtsc_ordered();
 	regs->ax = lower_32_bits(tsc);
 	regs->dx = upper_32_bits(tsc);
@@ -416,7 +417,7 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 	case MSR_IA32_TSC:
 	case MSR_AMD64_GUEST_TSC_FREQ:
 		if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
-			return __vc_handle_secure_tsc_msrs(regs, write);
+			return __vc_handle_secure_tsc_msrs(regs, write, ctxt);
 		break;
 	default:
 		break;

base-commit: 9f4a425223f3bd8ccaebc7f4f42b1d8c5f12fb45
-- 
2.43.0

Re: [PATCH v2] x86/sev: Improve handling of writes to intercepted TSC MSRs

Posted by Tom Lendacky 2 months, 3 weeks ago

On 7/16/25 00:53, Nikunj A Dadhania wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Currently, when a Secure TSC enabled SNP guest attempts to write to the
> intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel response
> incorrectly implies a VMM configuration error, when in fact it is the usual
> VMM configuration to intercept writes to read-only MSRs, unless explicitly
> documented.
> 
> Modify the intercepted TSC MSR #VC handling:
> * Write to GUEST_TSC_FREQ will generate a #GP instead of terminating the
>   guest
> * Write to MSR_IA32_TSC will generate a #GP instead of silently ignoring it
> 
> Add a WARN_ONCE to log the incident, as well-behaved SNP guest kernels
> should never attempt to write to these MSRs.
> 
> However, continue to terminate the guest when reading from intercepted
> GUEST_TSC_FREQ MSR with Secure TSC enabled, as intercepted reads indicate
> an improper VMM configuration for Secure TSC enabled SNP guests.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Co-developed-by: Nikunj A Dadhania <nikunj@amd.com>
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>  arch/x86/coco/sev/vc-handle.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
> index faf1fce89ed4..18be9f8bd015 100644
> --- a/arch/x86/coco/sev/vc-handle.c
> +++ b/arch/x86/coco/sev/vc-handle.c
> @@ -371,29 +371,30 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
>   * executing with Secure TSC enabled, so special handling is required for
>   * accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ.
>   */
> -static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write)
> +static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write,
> +						  struct es_em_ctxt  *ctxt)

The pt_regs pointer is part of the es_em_ctxt struct, so probably just
change this to:

static enum es_result __vc_handle_secure_tsc_msrs(struct es_em_ctxt *ctxt, bool write)

and then get the regs pointer from ctxt:

struct pt_regs *regs = ctxt->regs;

>  {
>  	u64 tsc;
>  
>  	/*
> -	 * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
> -	 * Terminate the SNP guest when the interception is enabled.
> +	 * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
> +	 * return undefined values, and GUEST_TSC_FREQ is read-only. Generate
> +	 * a #GP on all writes, but WARN to log a kernel bug.
>  	 */
> -	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
> -		return ES_VMM_ERROR;
> +	if (WARN_ON_ONCE(write)) {

Do we want to capture individual WARNs for each MSR? I guess I'm ok with
a single WARN for either MSR, but just asking the question.

Thanks,
Tom

> +		ctxt->fi.vector = X86_TRAP_GP;
> +		ctxt->fi.error_code = 0;
> +		return ES_EXCEPTION;
> +	}
>  
>  	/*
> -	 * Writes: Writing to MSR_IA32_TSC can cause subsequent reads of the TSC
> -	 *         to return undefined values, so ignore all writes.
> -	 *
> -	 * Reads: Reads of MSR_IA32_TSC should return the current TSC value, use
> -	 *        the value returned by rdtsc_ordered().
> +	 * GUEST_TSC_FREQ read should not be intercepted when Secure TSC is
> +	 * enabled. Terminate the SNP guest when the interception is enabled.
>  	 */
> -	if (write) {
> -		WARN_ONCE(1, "TSC MSR writes are verboten!\n");
> -		return ES_OK;
> -	}
> +	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
> +		return ES_VMM_ERROR;
>  
> +	/* Reads of MSR_IA32_TSC should return the current TSC value. */
>  	tsc = rdtsc_ordered();
>  	regs->ax = lower_32_bits(tsc);
>  	regs->dx = upper_32_bits(tsc);
> @@ -416,7 +417,7 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
>  	case MSR_IA32_TSC:
>  	case MSR_AMD64_GUEST_TSC_FREQ:
>  		if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
> -			return __vc_handle_secure_tsc_msrs(regs, write);
> +			return __vc_handle_secure_tsc_msrs(regs, write, ctxt);
>  		break;
>  	default:
>  		break;
> 
> base-commit: 9f4a425223f3bd8ccaebc7f4f42b1d8c5f12fb45

Re: [PATCH v2] x86/sev: Improve handling of writes to intercepted TSC MSRs

Posted by Sean Christopherson 2 months, 3 weeks ago

On Wed, Jul 16, 2025, Tom Lendacky wrote:
> On 7/16/25 00:53, Nikunj A Dadhania wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > Currently, when a Secure TSC enabled SNP guest attempts to write to the
> > intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel response
> > incorrectly implies a VMM configuration error, when in fact it is the usual
> > VMM configuration to intercept writes to read-only MSRs, unless explicitly
> > documented.
> > 
> > Modify the intercepted TSC MSR #VC handling:
> > * Write to GUEST_TSC_FREQ will generate a #GP instead of terminating the
> >   guest
> > * Write to MSR_IA32_TSC will generate a #GP instead of silently ignoring it
> > 
> > Add a WARN_ONCE to log the incident, as well-behaved SNP guest kernels
> > should never attempt to write to these MSRs.
> > 
> > However, continue to terminate the guest when reading from intercepted
> > GUEST_TSC_FREQ MSR with Secure TSC enabled, as intercepted reads indicate
> > an improper VMM configuration for Secure TSC enabled SNP guests.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>

Feel free to drop me as author and just give me a Reported-by or Suggested-by.
At this point, I ain't doing a whole lot of anything for this patch :-)

> > +	if (WARN_ON_ONCE(write)) {
> 
> Do we want to capture individual WARNs for each MSR? I guess I'm ok with
> a single WARN for either MSR, but just asking the question.

Or don't WARN at all.  If the caller is doing a bare wrmsrq(), then the kernel
will WARN in ex_handler_msr().  If the caller is doing wrmsrq_safe(), do we care
that they're being deliberately weird?

Re: [PATCH v2] x86/sev: Improve handling of writes to intercepted TSC MSRs

Posted by Nikunj A Dadhania 2 months, 2 weeks ago

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jul 16, 2025, Tom Lendacky wrote:
>> On 7/16/25 00:53, Nikunj A Dadhania wrote:
>> > From: Sean Christopherson <seanjc@google.com>
>> > 
>> > Currently, when a Secure TSC enabled SNP guest attempts to write to the
>> > intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel response
>> > incorrectly implies a VMM configuration error, when in fact it is the usual
>> > VMM configuration to intercept writes to read-only MSRs, unless explicitly
>> > documented.
>> > 
>> > Modify the intercepted TSC MSR #VC handling:
>> > * Write to GUEST_TSC_FREQ will generate a #GP instead of terminating the
>> >   guest
>> > * Write to MSR_IA32_TSC will generate a #GP instead of silently ignoring it
>> > 
>> > Add a WARN_ONCE to log the incident, as well-behaved SNP guest kernels
>> > should never attempt to write to these MSRs.
>> > 
>> > However, continue to terminate the guest when reading from intercepted
>> > GUEST_TSC_FREQ MSR with Secure TSC enabled, as intercepted reads indicate
>> > an improper VMM configuration for Secure TSC enabled SNP guests.
>> > 
>> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> Feel free to drop me as author and just give me a Reported-by or Suggested-by.
> At this point, I ain't doing a whole lot of anything for this patch :-)

Sure

>
>> > +	if (WARN_ON_ONCE(write)) {
>> 
>> Do we want to capture individual WARNs for each MSR? I guess I'm ok with
>> a single WARN for either MSR, but just asking the question.
>
> Or don't WARN at all.  If the caller is doing a bare wrmsrq(), then the kernel
> will WARN in ex_handler_msr().  If the caller is doing wrmsrq_safe(), do we care
> that they're being deliberately weird?

Agree, we can drop the WARN.

Regards,
Nikunj

Re: [PATCH v2] x86/sev: Improve handling of writes to intercepted TSC MSRs

Posted by Nikunj A Dadhania 2 months, 3 weeks ago

Tom Lendacky <thomas.lendacky@amd.com> writes:

> On 7/16/25 00:53, Nikunj A Dadhania wrote:
>> From: Sean Christopherson <seanjc@google.com>
>> 
>> Currently, when a Secure TSC enabled SNP guest attempts to write to the
>> intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel response
>> incorrectly implies a VMM configuration error, when in fact it is the usual
>> VMM configuration to intercept writes to read-only MSRs, unless explicitly
>> documented.
>> 
>> Modify the intercepted TSC MSR #VC handling:
>> * Write to GUEST_TSC_FREQ will generate a #GP instead of terminating the
>>   guest
>> * Write to MSR_IA32_TSC will generate a #GP instead of silently ignoring it
>> 
>> Add a WARN_ONCE to log the incident, as well-behaved SNP guest kernels
>> should never attempt to write to these MSRs.
>> 
>> However, continue to terminate the guest when reading from intercepted
>> GUEST_TSC_FREQ MSR with Secure TSC enabled, as intercepted reads indicate
>> an improper VMM configuration for Secure TSC enabled SNP guests.
>> 
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> Co-developed-by: Nikunj A Dadhania <nikunj@amd.com>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>  arch/x86/coco/sev/vc-handle.c | 31 ++++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 15 deletions(-)
>> 
>> diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
>> index faf1fce89ed4..18be9f8bd015 100644
>> --- a/arch/x86/coco/sev/vc-handle.c
>> +++ b/arch/x86/coco/sev/vc-handle.c
>> @@ -371,29 +371,30 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
>>   * executing with Secure TSC enabled, so special handling is required for
>>   * accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ.
>>   */
>> -static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write)
>> +static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write,
>> +						  struct es_em_ctxt  *ctxt)
>
> The pt_regs pointer is part of the es_em_ctxt struct, so probably just
> change this to:
>
> static enum es_result __vc_handle_secure_tsc_msrs(struct es_em_ctxt *ctxt, bool write)
>
> and then get the regs pointer from ctxt:
>
> struct pt_regs *regs = ctxt->regs;

Ack

>
>>  {
>>  	u64 tsc;
>>  
>>  	/*
>> -	 * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
>> -	 * Terminate the SNP guest when the interception is enabled.
>> +	 * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
>> +	 * return undefined values, and GUEST_TSC_FREQ is read-only. Generate
>> +	 * a #GP on all writes, but WARN to log a kernel bug.
>>  	 */
>> -	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
>> -		return ES_VMM_ERROR;
>> +	if (WARN_ON_ONCE(write)) {
>
> Do we want to capture individual WARNs for each MSR? I guess I'm ok with
> a single WARN for either MSR, but just asking the question.

Right, I had thought about this and concluded that a single WARN should
be fine. The chance of both the MSR being intercepted and written are
pretty low, and anyways a GP# will be generate for all the writes.

Regards,
Nikunj