linux-kernel - Re: [PATCH] x86/debug: Fix stack recursion caused by DR7 accesses

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <357d5157e01ee4469ca38595e941971083a51212.camel@gmail.com>
Date:   Mon, 30 Jan 2023 11:25:44 +0100
From:   Miko Larsson <mikoxyzzz@...il.com>
To:     Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     x86@...nel.org, hpa@...or.com,
        Sean Christopherson <seanjc@...gle.com>, peterz@...radead.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Joerg Roedel <jroedel@...e.de>,
        Alexey Kardashevskiy <aik@....com>
Subject: Re: [PATCH] x86/debug: Fix stack recursion caused by DR7 accesses

On Mon, 2023-01-30 at 10:37 +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@...e.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@....com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Signed-off-by: Joerg Roedel <jroedel@...e.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