linux-kernel - Re: [PATCH v3 06/21] x86/virt/tdx: Shut down TDX module in case of error

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: <6b9d89e1bd4d9991b0cdb778224474696e8e6b67.camel@intel.com>
Date:   Thu, 19 May 2022 11:51:52 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Sagi Shahar <sagis@...gle.com>
Cc:     Dave Hansen <dave.hansen@...el.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>, len.brown@...el.com,
        tony.luck@...el.com, rafael.j.wysocki@...el.com,
        reinette.chatre@...el.com, dan.j.williams@...el.com,
        peterz@...radead.org, ak@...ux.intel.com,
        kirill.shutemov@...ux.intel.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v3 06/21] x86/virt/tdx: Shut down TDX module in case of
 error

On Wed, 2022-05-18 at 09:19 -0700, Sagi Shahar wrote:
> On Tue, Apr 26, 2022 at 5:06 PM Kai Huang <kai.huang@...el.com> wrote:
> > 
> > On Tue, 2022-04-26 at 13:59 -0700, Dave Hansen wrote:
> > > On 4/5/22 21:49, Kai Huang wrote:
> > > > TDX supports shutting down the TDX module at any time during its
> > > > lifetime.  After TDX module is shut down, no further SEAMCALL can be
> > > > made on any logical cpu.
> > > 
> > > Is this strictly true?
> > > 
> > > I thought SEAMCALLs were used for the P-SEAMLDR too.
> > 
> > Sorry will change to no TDX module SEAMCALL can be made on any logical cpu.
> > 
> > [...]
> > 
> > > > 
> > > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */
> > > > +struct seamcall_ctx {
> > > > +   u64 fn;
> > > > +   u64 rcx;
> > > > +   u64 rdx;
> > > > +   u64 r8;
> > > > +   u64 r9;
> > > > +   atomic_t err;
> > > > +   u64 seamcall_ret;
> > > > +   struct tdx_module_output out;
> > > > +};
> > > > +
> > > > +static void seamcall_smp_call_function(void *data)
> > > > +{
> > > > +   struct seamcall_ctx *sc = data;
> > > > +   int ret;
> > > > +
> > > > +   ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9,
> > > > +                   &sc->seamcall_ret, &sc->out);
> 
> Are the seamcall_ret and out fields in seamcall_ctx going to be used?
> Right now it looks like no one is going to read them.
> If they are going to be used then this is going to cause a race since
> the different CPUs are going to write concurrently to the same address
> inside seamcall().
> We should either use local memory and write using atomic_set like the
> case for the err field or hard code NULL at the call site if they are
> not going to be used.
> > > > 

Thanks for catching this.  Both 'seamcall_ret' and 'out' are actually not used
in this series, but this needs to be improved for sure.  

I think I can just remove them from the 'seamcall_ctx' for now, since they are
not used at all.

-- 
Thanks,
-Kai