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

[David Laight]

From: Joerg Roedel
> Sent: 30 January 2023 09:37
> 
> From: Joerg Roedel <jroedel@xxxxxxx>
> 
> 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@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
> ---
>  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)