1. Patchew
  2. linux
  3. View series

[PATCH] x86/debug: Fix stack recursion caused by DR7 accesses

Joerg Roedel posted 1 patch 2 years, 7 months ago
There is a newer version of this series
arch/x86/include/asm/debugreg.h | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
[PATCH] x86/debug: Fix stack recursion caused by DR7 accesses
Posted by Joerg Roedel 2 years, 7 months ago 
From: Joerg Roedel <jroedel@suse.de>

In kernels compiled with CONFIG_PARAVIRT=n the compiler re-orders the
DR7 read in exc_nmi() to happen before the call to sev_es_ist_enter().

This is problematic when running as an SEV-ES guest because in this
environemnt the DR7 read might cause a #VC exception, and taking #VC
exceptions is not safe in exc_nmi() before sev_es_ist_enter() has run.

The result is stack recursion if the NMI was caused on the #VC IST
stack, because a subsequent #VC exception in the NMI handler will
overwrite the stack frame of the interrupted #VC handler.

As there are no compiler barriers affecting the ordering of DR7
reads/writes, make the accesses to this register volatile, forbidding
the compiler to re-order them.

Cc: Alexey Kardashevskiy <aik@amd.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/include/asm/debugreg.h | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index b049d950612f..eb6238a5f60c 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -39,7 +39,18 @@ static __always_inline unsigned long native_get_debugreg(int regno)
 		asm("mov %%db6, %0" :"=r" (val));
 		break;
 	case 7:
-		asm("mov %%db7, %0" :"=r" (val));
+		/*
+		 * Make DR7 reads volatile to forbid re-ordering them with other
+		 * code. This is needed because a DR7 access can cause a #VC
+		 * exception when running under SEV-ES. But taking a #VC
+		 * exception is not safe at everywhere in the code-flow and
+		 * re-ordering might place the access into an unsafe place.
+		 *
+		 * This happened in the NMI handler, where the DR7 read was
+		 * re-ordered to happen before the call to sev_es_ist_enter(),
+		 * causing stack recursion.
+		 */
+		asm volatile ("mov %%db7, %0" : "=r" (val));
 		break;
 	default:
 		BUG();
@@ -66,7 +77,21 @@ static __always_inline void native_set_debugreg(int regno, unsigned long value)
 		asm("mov %0, %%db6"	::"r" (value));
 		break;
 	case 7:
-		asm("mov %0, %%db7"	::"r" (value));
+		/*
+		 * Make DR7 writes volatile to forbid re-ordering them with
+		 * other code. This is needed because a DR7 access can cause a
+		 * #VC exception when running under SEV-ES.  But taking a #VC
+		 * exception is not safe at everywhere in the code-flow and
+		 * re-ordering might place the access into an unsafe place.
+		 *
+		 * This happened in the NMI handler, where the DR7 read was
+		 * re-ordered to happen before the call to sev_es_ist_enter(),
+		 * causing stack recursion.
+		 *
+		 * While is didn't happen with a DR7 write, add the volatile
+		 * here too to avoid similar problems in the future.
+		 */
+		asm volatile ("mov %0, %%db7"	::"r" (value));
 		break;
 	default:
 		BUG();
-- 
2.39.0
RE: [PATCH] x86/debug: Fix stack recursion caused by DR7 accesses
Posted by David Laight 2 years, 7 months ago 
From: Joerg Roedel
> Sent: 30 January 2023 09:37
> 
> From: Joerg Roedel <jroedel@suse.de>
> 
> In kernels compiled with CONFIG_PARAVIRT=n the compiler re-orders the
> DR7 read in exc_nmi() to happen before the call to sev_es_ist_enter().

More the case that you happen to be 'unlucky' in this case.

> This is problematic when running as an SEV-ES guest because in this
> environemnt the DR7 read might cause a #VC exception, and taking #VC
> exceptions is not safe in exc_nmi() before sev_es_ist_enter() has run.
> 
> The result is stack recursion if the NMI was caused on the #VC IST
> stack, because a subsequent #VC exception in the NMI handler will
> overwrite the stack frame of the interrupted #VC handler.
> 
> As there are no compiler barriers affecting the ordering of DR7
> reads/writes, make the accesses to this register volatile, forbidding
> the compiler to re-order them.

Is that enough?
IIRC 'asm volatile' are only ordered w.r.t other 'asm volatile'.
To stop normal memory accesses being re-ordered you need a "memory" clobber.

All cpu registers are effectively memory, you should be able to use
partial memory clobber for any without side effects.
But if they have side effects on any other memory (or cpu register)
accesses I'd have thought you pretty much need a full compiler
memory barrier.

For most code the cost of a full compiler memory barrier is likely
to be limited.

	David

> 
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/include/asm/debugreg.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index b049d950612f..eb6238a5f60c 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -39,7 +39,18 @@ static __always_inline unsigned long native_get_debugreg(int regno)
>  		asm("mov %%db6, %0" :"=r" (val));
>  		break;
>  	case 7:
> -		asm("mov %%db7, %0" :"=r" (val));
> +		/*
> +		 * Make DR7 reads volatile to forbid re-ordering them with other
> +		 * code. This is needed because a DR7 access can cause a #VC
> +		 * exception when running under SEV-ES. But taking a #VC
> +		 * exception is not safe at everywhere in the code-flow and
> +		 * re-ordering might place the access into an unsafe place.
> +		 *
> +		 * This happened in the NMI handler, where the DR7 read was
> +		 * re-ordered to happen before the call to sev_es_ist_enter(),
> +		 * causing stack recursion.
> +		 */
> +		asm volatile ("mov %%db7, %0" : "=r" (val));
>  		break;
>  	default:
>  		BUG();
> @@ -66,7 +77,21 @@ static __always_inline void native_set_debugreg(int regno, unsigned long value)
>  		asm("mov %0, %%db6"	::"r" (value));
>  		break;
>  	case 7:
> -		asm("mov %0, %%db7"	::"r" (value));
> +		/*
> +		 * Make DR7 writes volatile to forbid re-ordering them with
> +		 * other code. This is needed because a DR7 access can cause a
> +		 * #VC exception when running under SEV-ES.  But taking a #VC
> +		 * exception is not safe at everywhere in the code-flow and
> +		 * re-ordering might place the access into an unsafe place.
> +		 *
> +		 * This happened in the NMI handler, where the DR7 read was
> +		 * re-ordered to happen before the call to sev_es_ist_enter(),
> +		 * causing stack recursion.
> +		 *
> +		 * While is didn't happen with a DR7 write, add the volatile
> +		 * here too to avoid similar problems in the future.
> +		 */
> +		asm volatile ("mov %0, %%db7"	::"r" (value));
>  		break;
>  	default:
>  		BUG();
> --
> 2.39.0

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Re: [PATCH] x86/debug: Fix stack recursion caused by DR7 accesses
Posted by Joerg Roedel 2 years, 7 months ago 
On Mon, Jan 30, 2023 at 10:37:17AM +0100, Joerg Roedel wrote:
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

Missed these tags:

Fixes: 315562c9af3d ("x86/sev-es: Adjust #VC IST Stack on entering NMI handler")
Cc: stable@vger.kernel.org

Regards,

-- 
Jörg Rödel
jroedel@suse.de

SUSE Software Solutions Germany GmbH
Frankenstraße 146
90461 Nürnberg
Germany

(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
Re: [PATCH] x86/debug: Fix stack recursion caused by DR7 accesses
Posted by Miko Larsson 2 years, 7 months ago 
On Mon, 2023-01-30 at 10:37 +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> In kernels compiled with CONFIG_PARAVIRT=n the compiler re-orders the
> DR7 read in exc_nmi() to happen before the call to
> sev_es_ist_enter().
> 
> This is problematic when running as an SEV-ES guest because in this
> environemnt the DR7 read might cause a #VC exception, and taking #VC
> exceptions is not safe in exc_nmi() before sev_es_ist_enter() has
> run.
> 
> The result is stack recursion if the NMI was caused on the #VC IST
> stack, because a subsequent #VC exception in the NMI handler will
> overwrite the stack frame of the interrupted #VC handler.
> 
> As there are no compiler barriers affecting the ordering of DR7
> reads/writes, make the accesses to this register volatile, forbidding
> the compiler to re-order them.
> 
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/include/asm/debugreg.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/debugreg.h
> b/arch/x86/include/asm/debugreg.h
> index b049d950612f..eb6238a5f60c 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -39,7 +39,18 @@ static __always_inline unsigned long
> native_get_debugreg(int regno)
>                 asm("mov %%db6, %0" :"=r" (val));
>                 break;
>         case 7:
> -               asm("mov %%db7, %0" :"=r" (val));
> +               /*
> +                * Make DR7 reads volatile to forbid re-ordering them
> with other
> +                * code. This is needed because a DR7 access can
> cause a #VC
> +                * exception when running under SEV-ES. But taking a
> #VC
> +                * exception is not safe at everywhere in the code-
> flow and
> +                * re-ordering might place the access into an unsafe
> place.
> +                *
> +                * This happened in the NMI handler, where the DR7
> read was
> +                * re-ordered to happen before the call to
> sev_es_ist_enter(),
> +                * causing stack recursion.
> +                */
> +               asm volatile ("mov %%db7, %0" : "=r" (val));
>                 break;
>         default:
>                 BUG();
> @@ -66,7 +77,21 @@ static __always_inline void
> native_set_debugreg(int regno, unsigned long value)
>                 asm("mov %0, %%db6"     ::"r" (value));
>                 break;
>         case 7:
> -               asm("mov %0, %%db7"     ::"r" (value));
> +               /*
> +                * Make DR7 writes volatile to forbid re-ordering
> them with
> +                * other code. This is needed because a DR7 access
> can cause a
> +                * #VC exception when running under SEV-ES.  But
> taking a #VC
> +                * exception is not safe at everywhere in the code-
> flow and
> +                * re-ordering might place the access into an unsafe
> place.
> +                *
> +                * This happened in the NMI handler, where the DR7
> read was
> +                * re-ordered to happen before the call to
> sev_es_ist_enter(),
> +                * causing stack recursion.
> +                *
> +                * While is didn't happen with a DR7 write, add the
> volatile
> +                * here too to avoid similar problems in the future.
> +                */
> +               asm volatile ("mov %0, %%db7"   ::"r" (value));
>                 break;
>         default:
>                 BUG();

This should probably be Cc'ed to the stable mailing list.
-- 
~miko

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.