linux-kernel - Re: [PATCH v11 18/20] x86: Handle TDX erratum to reset TDX private memory during kexec() and reboot

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: <0be5634f390015ee6badb3f2b2154ad90eb70434.camel@intel.com>
Date:   Tue, 20 Jun 2023 00:56:42 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Hansen, Dave" <dave.hansen@...el.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Luck, Tony" <tony.luck@...el.com>,
        "david@...hat.com" <david@...hat.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v11 18/20] x86: Handle TDX erratum to reset TDX private
 memory during kexec() and reboot

On Mon, 2023-06-19 at 16:41 -0700, Dave Hansen wrote:
> On 6/19/23 07:46, kirill.shutemov@...ux.intel.com wrote:
> > > > 
> > > > Using atomic_set() requires changing tdmr->pamt_4k_base to atomic_t, which is a
> > > > little bit silly or overkill IMHO.  Looking at the code, it seems
> > > > arch_atomic_set() simply uses __WRITE_ONCE():
> > > How about _adding_ a variable that protects tdmr->pamt_4k_base?
> > > Wouldn't that be more straightforward than mucking around with existing
> > > types?
> > What's wrong with simple global spinlock that protects all tdmr->pamt_*?
> > It is much easier to follow than a custom serialization scheme.
> 
> Quick, what prevents a:
> 
> 	spin_lock() => #MC => spin_lock()
> 
> deadlock?
> 
> Plain old test/sets don't deadlock ever.

Agreed.  So I think having any locking in #MC handle is kinda dangerous. 

Adding "a" variable has another advantage:  We can have a more precise result of
whether we need to reset PAMT pages, even those PAMTs are already allocated and
set to the TDMRs, because the TDX module only starts to write PAMTs using global
KeyID until some SEAMCALL.

Any comments to below?

+static bool tdx_private_mem_begin;
+
 /*
  * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
  * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
@@ -1141,6 +1143,8 @@ static int init_tdx_module(void)
         */
        wbinvd_on_all_cpus();
 
+       WRITE_ONCE(tdx_private_mem_begin, true);
+
        /* Config the key of global KeyID on all packages */
        ret = config_global_keyid();
        if (ret)
@@ -1463,6 +1467,14 @@ static void tdx_memory_shutdown(void)
         */
        WARN_ON_ONCE(num_online_cpus() != 1);
 
+       /*
+        * It's not possible to have any TDX private pages if the TDX
+        * module hasn't started to write any memory using the global
+        * KeyID.
+        */
+       if (!READ_ONCE(tdx_private_mem_begin))
+               return;
+
        tdmrs_reset_pamt_all(&tdx_tdmr_list);